Skip to content

Conversation

@martinjrobins
Copy link
Contributor

Description

Updates EvaluateAt so that it can have a child that is evaluated at edges

Fixes # (issue)

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.79%. Comparing base (f1ceff2) to head (491219e).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5190   +/-   ##
========================================
  Coverage    98.79%   98.79%           
========================================
  Files          320      320           
  Lines        27092    27096    +4     
========================================
+ Hits         26766    26770    +4     
  Misses         326      326           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@martinjrobins martinjrobins marked this pull request as ready for review September 17, 2025 14:22
@martinjrobins martinjrobins requested a review from a team as a code owner September 17, 2025 14:22
@rtimms
Copy link
Contributor

rtimms commented Sep 17, 2025

@Dharshannan does this fix your model?

@Dharshannan
Copy link

Yeap this fix works, thanks.
Separately could we create another PR for adding the IDA solver to be used in initial root-finding apart from only Casadi (this is not too important but would be nice to have to test a DAE index 2 system).

@martinjrobins
Copy link
Contributor Author

Yeap this fix works, thanks. Separately could we create another PR for adding the IDA solver to be used in initial root-finding apart from only Casadi (this is not too important but would be nice to have to test a DAE index 2 system).

yes can you please add another issue for this

brosaplanella
brosaplanella previously approved these changes Sep 29, 2025
Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Looks good! We probably need an entry in the CHANGELOG for this.

Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Small comment in the CHANGELOG

martinjrobins and others added 2 commits October 1, 2025 08:25
@brosaplanella
Copy link
Member

@martinjrobins I am happy with it, but my approval doesn't work because I made the last push. Not sure if you can approve.

@Dharshannan Dharshannan self-requested a review October 6, 2025 10:51
Copy link

@Dharshannan Dharshannan left a comment

Choose a reason for hiding this comment

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

LGTM

@Dharshannan
Copy link

@martinjrobins and @brosaplanella I can't approve as I do not have write access. @rtimms could you approve this?

@martinjrobins martinjrobins merged commit 5d1ee71 into develop Oct 7, 2025
51 of 54 checks passed
@martinjrobins martinjrobins deleted the feat-eval-at-edge branch October 7, 2025 15:43
pipliggins pushed a commit to pipliggins/PyBaMM that referenced this pull request Oct 13, 2025
…-team#5190)

* feat: EvaluateAt: add support for children evaluated at edges

* add test for evaluate at edges

* coverage

* add changelog

---------

Co-authored-by: Ferran Brosa Planella <[email protected]>
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.

5 participants