-
Notifications
You must be signed in to change notification settings - Fork 92
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
Fix initial conditions #1364
Fix initial conditions #1364
Conversation
…ion procedure to allow evaluation from beginning.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
I really like the changes made here, good job @vlipovac! Please see my only concern in the review.
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.
I post various comments here, but need to think more about the super-chain.
Co-authored-by: Eirik Keilegavlen <[email protected]>
…into fix_initial_conditions
Looks good to me, great job @vlipovac ! |
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.
Nice work, @vlipovac! This makes much more sense now, and as you pointed out yesterday, the PR is much more managable now.
My main concerns have been addressed now, also regarding the super calls. I left some comments and suggestions. In my estimate, these should all be rather minor; please let me know if I underestimate something - then I misunderstood, and we should perhaps discuss before you dive into a not-small change.
Co-authored-by: Eirik Keilegavlen <[email protected]>
…into fix_initial_conditions
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.
I think we are good now. Thanks for the effort and patience, @vlipovac
Closes #1344
Closes #1324
Proposed changes
Fixes the shortcomings discovered by @Yuriyzabegaev and implements the changes summarized in #1324, which was opened after #1236.
@keileg regarding our discussion about MRO issues and the usage of
super
, I have added a test which proofs that this is not the case for the initial condition framework. Hence it holds by extension for setting of variables, equations and BC (analogous usage ofsuper
), though not explicitly covered by tests.Types of changes
Checklist
pytest
was run with the--run-skipped
flag.