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

Fix initial conditions #1364

Merged
merged 28 commits into from
Mar 24, 2025
Merged

Fix initial conditions #1364

merged 28 commits into from
Mar 24, 2025

Conversation

vlipovac
Copy link
Contributor

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 of super), though not explicitly covered by tests.

Types of changes

  • Minor change (e.g., dependency bumps, broken links).
  • Bugfix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Testing (contribution related to testing of existing or new functionality).
  • Documentation (contribution related to adding, improving, or fixing documentation).
  • Maintenance (e.g., improve logic and performance, remove obsolete code).
  • Other:

Checklist

  • The documentation is up-to-date.
  • Static typing is included in the update.
  • This PR does not duplicate existing functionality.
  • The update is covered by the test suite (including tests added in the PR).
  • If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@keileg keileg self-assigned this Feb 28, 2025
Copy link
Contributor

@Yuriyzabegaev Yuriyzabegaev left a 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.

Copy link
Contributor

@keileg keileg left a 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.

@vlipovac vlipovac requested review from keileg and Yuriyzabegaev March 4, 2025 13:39
@Yuriyzabegaev
Copy link
Contributor

Looks good to me, great job @vlipovac !

Copy link
Contributor

@keileg keileg left a 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.

@vlipovac vlipovac requested a review from keileg March 19, 2025 11:37
Copy link
Contributor

@keileg keileg left a 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

@vlipovac vlipovac merged commit e28f4d8 into develop Mar 24, 2025
6 checks passed
@vlipovac vlipovac deleted the fix_initial_conditions branch March 24, 2025 13:47
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.

Bug in initial conditions Revisit the setting of initial conditions
4 participants