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

Use more abstract types to create a hierarchy #15

Merged
merged 2 commits into from
Aug 14, 2023
Merged

Conversation

Vaibhavdixit02
Copy link
Member

No description provided.

Copy link

@ai-maintainer ai-maintainer bot left a comment

Choose a reason for hiding this comment

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

AI-Maintainer Review for PR - Use more abstract types to create a hierarchy

Title and Description ⚠️

The title is clear but the description is missing

The title of the pull request is clear and provides a concise summary of the changes. However, the description is missing. It would be beneficial to include a description that provides more context and explains the rationale for using more abstract types and creating a hierarchy.

Code Changes 👍

The changes are narrowly focused and relevant

The changes in the pull request are narrowly focused on introducing more abstract types to create a hierarchy. The diff primarily consists of the addition of new abstract types and their respective subtypes, along with a few modifications to existing types. There are no indications of the author attempting to resolve multiple issues simultaneously.

Testing ⚠️

Information about testing is missing

The description does not provide any information about how the changes were tested. It would be beneficial to include details about the testing approach taken to ensure the correctness and functionality of the introduced abstract types and their subtypes.

Documentation ⚠️

Docstrings for new types are missing

The newly added abstract types and their subtypes do not have docstrings. It would be beneficial to include docstrings that describe their behavior, arguments, and return values.

Suggested Changes

  • Please add a description that provides more context and explains the rationale for the changes.
  • Please provide information about how the changes were tested.
  • Please add docstrings for the newly added abstract types and their subtypes.

Reviewed with AI Maintainer

src/ADTypes.jl Outdated
Comment on lines 52 to 56
struct AutoEnzyme{M} <: AbstractADType
mode::M
end

AutoEnzyme(; mode = nothing) = AutoEnzyme(mode)
Copy link
Member

Choose a reason for hiding this comment

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

Should we split Enzyme into forward and reverse modes to fit into this better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was conflicted about this as well. The mode field should take the choice per the current definition. That can be the Enzyme Forward and Reverse objects directly, it's nothing because a default mode doesn't make sense to me it would be completely up the package if they want to put a default mode.

src/ADTypes.jl Outdated
struct AutoZygote <: AbstractADType end
struct AutoZygote <: AbstractReverseMode end

struct AutoSparseZygote <: AbstractSparseReverseMode end

struct AutoEnzyme{M} <: AbstractADType
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
struct AutoEnzyme{M} <: AbstractADType
struct AutoEnzyme{M} <: Union{AutoForwardMode, AutoReverseMode, AutoSparseForwardMode, AutoSparseReverseMode}

Copy link
Member Author

@Vaibhavdixit02 Vaibhavdixit02 Aug 12, 2023

Choose a reason for hiding this comment

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

This could be an option to keep dispatches happy 😅 but will put all the onus on the downstream package developer to make sure it's used with M being checked

@Vaibhavdixit02 Vaibhavdixit02 mentioned this pull request Aug 12, 2023
Vaibhavdixit02 and others added 2 commits August 14, 2023 16:18
Co-authored-by: Avik Pal <avik.pal.2017@gmail.com>
@Vaibhavdixit02 Vaibhavdixit02 merged commit 8e141dc into main Aug 14, 2023
3 checks passed
@Vaibhavdixit02 Vaibhavdixit02 deleted the hierarchy branch August 14, 2023 11:03
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.

None yet

2 participants