Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import to Tape #50

Merged
merged 15 commits into from
Sep 11, 2021
Merged

Import to Tape #50

merged 15 commits into from
Sep 11, 2021

Conversation

dfdx
Copy link
Collaborator

@dfdx dfdx commented Aug 19, 2021

See the discussion in #49

@dfdx dfdx mentioned this pull request Aug 19, 2021
@dfdx dfdx changed the title [WIP] Import to Tape Import to Tape Sep 2, 2021
src/ops.jl Show resolved Hide resolved
src/ops.jl Show resolved Hide resolved
src/ops.jl Outdated Show resolved Hide resolved
test/ort.jl Show resolved Hide resolved
src/load.jl Outdated Show resolved Hide resolved
Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Some minor nitpicks, but the one comment I had on the core of this PR was the approach to handling "missing rules" with load_node!. I think it's possible to do this with a default dispatch that returns missing, and it would be a more "Julian" approach.

src/load.jl Show resolved Hide resolved
src/load.jl Show resolved Hide resolved
src/load.jl Outdated Show resolved Hide resolved
src/load.jl Outdated Show resolved Hide resolved
src/load.jl Outdated Show resolved Hide resolved
src/ops.jl Outdated Show resolved Hide resolved
src/ops.jl Outdated Show resolved Hide resolved
Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, we can tackle some of the other suggestions in future PRs!

@darsnack
Copy link
Member

darsnack commented Sep 4, 2021

@ayush-1506 Since the codebase has changed significantly, would it make sense to grant write access to FluxML committers on this repo who are tracking this work (@ToucheSir and myself)?

@ayush-1506
Copy link
Member

@darsnack Yes, unfortunately I don't have the required rights to grant write access to others. Tagging @DhairyaLGandhi here.

@DhairyaLGandhi
Copy link
Member

Will do so.

@darsnack
Copy link
Member

@DhairyaLGandhi Bump

@DhairyaLGandhi
Copy link
Member

Done

@darsnack darsnack merged commit 57af573 into FluxML:master Sep 11, 2021
@dfdx dfdx deleted the dfdx/tape branch September 11, 2021 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants