-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add rules for all Symmetric/Hermitian constructors #182
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
fecbf59
Expand rule for lower symmetric constructor
sethaxen ad6bf62
Expand tests to lower and complex
sethaxen a4a3392
Add rule for Hermitian constructors
sethaxen 9ef348e
Merge branch 'master' into symherm
sethaxen 9f944ae
Unify Symmetric and Hermitian rules
sethaxen d01b959
Unify symmetric and hermitian tests
sethaxen 19067b1
Add frule
sethaxen 55ad950
Reformat
sethaxen cf6ff85
Add rule for conversion to matrix
sethaxen 58897c1
Add rrule for Array
sethaxen da14328
Add frules for Array and Matrix
sethaxen 4e764f1
Increment version number
sethaxen cec9eea
Merge branch 'master' into symherm
sethaxen 6881ae1
Increment version number
sethaxen 6f9ddbf
Add methods with matrix args
sethaxen b76b62c
Dispatch on realness
sethaxen f360868
Call Matrix instead of collect
sethaxen f80788a
Add coments
sethaxen 341ac0b
Remove type constraints
sethaxen fc99f7c
Apply suggestions from code review
sethaxen 51caf2e
Wrap lines
sethaxen 6f79df0
Use more informative type names
sethaxen 0557ebb
Merge branch 'master' into symherm
sethaxen 6b2c7e9
Increment version number
sethaxen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a thunk here? The cotangent w.r.t. A is the only meaningful, so I would imagine that it would always get used somewhere, but my intuition for this isn't great, perhaps yours is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not? I had thought that the convention was to always thunk in reverse unless 1) there's only one cotangent (not counting the
NO_FIELDS
) or 2) sometimes for scalar functions (e.g. those made with@scalar_rule
). I personally think modifying (1) to be "there's only one cotangent that a user could reasonably want" makes perfect sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oxinabox do you agree? If so I'll open a PR to the docs to update guidance.