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

Test for AllenCahn_1D_FD.py #370

Merged
merged 8 commits into from
Oct 27, 2023
Merged

Conversation

lisawim
Copy link
Collaborator

@lisawim lisawim commented Oct 27, 2023

I added a test for the one-dimensional AllenCahn equation. The test is more or less based on test_NonlinearSchroedinger_MPIFFT.py, testing things like:

  • Do right-hand side evaluations match in semi-implicit and fully-implicit case?,
  • check errors for each problem classes,
  • do number of function calls match in semi-implicit and fully-implicit case?,
  • is ProblemError raised correctly when nan arises?,
  • and are logger.warning also called correctly in case of nan

Such basic things. In my opinion there could be also tests for some problem specific things. But at this moment I don't know yet which things this could be. For example, one of the AC-classes uses Finel's trick, which is only a rewriting of the right-hand side. Here could be also a specific test added, but I don't have any idea what test would make sense.

Let me know if anyone has some idea what I can add.

Copy link
Contributor

@brownbaerchen brownbaerchen left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks like you fixed quite a lot of things!

Generally, a test that I would find useful for problems with a Newton solver is to compare the solver to Newton Krylov from scipy like implemented here.

It should be possible to write a generic function that takes as input a problem class and some parameters and then solves using the solver in the problem class and scipy and compares. It is some effort, though. So don't feel like you need to do this. But if you are eager to write some more tests, this could be useful.


if np.isnan(res) and self.stop_at_nan:
raise ProblemError('Newton got nan after %i iterations, aborting...' % n)
elif np.isnan(res):
self.logger.warning('Newton got nan after %i iterations...' % n)

print(f'{type(self).__name__}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, did forget that.

@@ -735,7 +717,7 @@ def __init__(
stop_at_nan=True,
):
super().__init__(nvars, dw, eps, newton_maxiter, newton_tol, interval, radius, stop_at_nan)
self.A -= sp.eye(self.init) * 0.0 / self.eps**2
self.A -= sp.eye(self.nvars) * 0.0 / self.eps**2
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this line do anything? It just subtracts zero. At least with your fix this does not raise an error, but still. Is there a reason for this line @pancetta?

Copy link
Member

Choose a reason for hiding this comment

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

There was, but not anymore. I don't know why this is still in there. Can be removed!

@@ -822,7 +805,7 @@ def __init__(
stop_at_nan=True,
):
super().__init__(nvars, dw, eps, newton_maxiter, newton_tol, interval, radius, stop_at_nan)
self.A -= sp.eye(self.init) * 0.0 / self.eps**2
self.A -= sp.eye(nvars) * 0.0 / self.eps**2
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, what does this do?

Copy link
Member

Choose a reason for hiding this comment

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

see above

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7142713) 72.66% compared to head (1e3930b) 73.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
+ Coverage   72.66%   73.62%   +0.96%     
==========================================
  Files         270      270              
  Lines       22387    22383       -4     
==========================================
+ Hits        16267    16480     +213     
+ Misses       6120     5903     -217     
Files Coverage Δ
...implementations/problem_classes/AllenCahn_1D_FD.py 95.51% <93.33%> (+95.51%) ⬆️

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

@lisawim
Copy link
Collaborator Author

lisawim commented Oct 27, 2023

Thanks for your comment! That sounds meaningful!

I also noticed that I forgot several lines to test. I'll add some more testing for allencahn_periodic_multiimplicit.

@brownbaerchen
Copy link
Contributor

I advocate the use of git hooks to automate black as described here.

@pancetta pancetta merged commit 73cb8e3 into Parallel-in-Time:master Oct 27, 2023
25 checks passed
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