-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fixes issue #234 using same inputs #369
Conversation
@ven-k there's now additional tests failing. I'll need to debug first. |
@ven-k all tests pass again |
Update based on documentation build fix
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.
Not strictly necessary, but I would suggest merging first two commits as they don't stand independently.
In future PRs I'd suggest "Update with rebase" option instead of merging main (for cleaner git history).
I have the foll. question, otherwise this looks good.
Project.toml
Outdated
@@ -30,7 +30,7 @@ PreallocationTools = "0.4.23" | |||
SafeTestsets = "0.1" | |||
SciMLStructures = "1.4.2" | |||
SymbolicIndexingInterface = "0.3.28" | |||
Symbolics = "6.14" | |||
Symbolics = "=6.29.2" |
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.
Why is this pinned to a version?
If at all this is only to set a lower bound, it would be better to not add =
. These tend to constraint package resolutions while using with other packages.
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.
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.
Symbolics 6.30 is yanked from the registry now. The pin can be removed.
@ven-k all done |
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.
The failing unit-tests are unrelated to this PR.
(Note that formatter failure is related)
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Output before:
Output after:
This MWE has been added as a test, which passes