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 BoundaryAdaptedGrid for only one central point #281

Merged
merged 6 commits into from
Jul 25, 2024

Conversation

JoshuaLampert
Copy link
Contributor

This fixes two independent, but related issues regarding the BoundaryAdaptedGrid:

  1. Previously the following failed:
D = derivative_operator(MattssonAlmquistVanDerWeide2018Minimal(), 1, 4, 0.0, 1.0, 9)

due to ERROR: ArgumentError: range(0.4999999999999999, stop=0.5, length=1): endpoints differ. This is because there is only one inner point and due to rounding errors the left and right ends are not the same. I simply added an if-statement for this. Let me know if you have a better idea to handle this @ranocha.

  1. With this fix, the above code (or, e.g., also D = derivative_operator(MattssonAlmquistVanDerWeide2018Accurate(), 1, 4, 0.0, 1.0, 11) already without the fix above) gives a result, but step(grid) gives zero, again, because there is only one point in the uniform_grid. This results in an operator with mass_matrix equal to zero and Infs and NaNs for the entries of Matrix(D). This modifies the step function for the BoundaryAdaptedGrid to return the actual step also for this edge case.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.59%. Comparing base (560c6d8) to head (cdc5575).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #281   +/-   ##
=======================================
  Coverage   91.58%   91.59%           
=======================================
  Files          35       35           
  Lines        5362     5364    +2     
=======================================
+ Hits         4911     4913    +2     
  Misses        451      451           
Flag Coverage Δ
unittests 91.59% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@coveralls
Copy link

coveralls commented Jul 23, 2024

Coverage Status

coverage: 92.362% (+0.003%) from 92.359%
when pulling cdc5575 on JoshuaLampert:fix-boundaryadaptedgrid
into 560c6d8 on ranocha:main.

Copy link
Owner

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks. I don't have a better idea than an if statement for the special case.

@ranocha
Copy link
Owner

ranocha commented Jul 24, 2024

Could you please bump the version in Project.toml so that I can make a new release directly after merging this?

@JoshuaLampert
Copy link
Contributor Author

Could you please bump the version in Project.toml so that I can make a new release directly after merging this?

Done in ef87f47.

@JoshuaLampert
Copy link
Contributor Author

Should I add a test for the case that N - 2M == 1? And if yes, is it enough to test that it successfully creates the operator?

@ranocha
Copy link
Owner

ranocha commented Jul 24, 2024

Adding a test would be great 👍 The test should also cover the second issue that you have described

This results in an operator with mass_matrix equal to zero and Infs and NaNs for the entries of Matrix(D).

@JoshuaLampert
Copy link
Contributor Author

I added a test in 683dfc9.

Copy link
Owner

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks!

@ranocha ranocha merged commit 9dc6ca8 into ranocha:main Jul 25, 2024
21 checks passed
@JoshuaLampert JoshuaLampert deleted the fix-boundaryadaptedgrid branch July 25, 2024 13:49
This pull request was closed.
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.

3 participants