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

Remove iteration counter and norms that are already covered by SolverStatistics #1232

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

pschultzendorff
Copy link
Contributor

@pschultzendorff pschultzendorff commented Sep 27, 2024

Changes proposed by @IngridKJ and I.

Proposed changes

Adresses issues #1219 and #1200.

model.nonlinar_solver_statistics already keeps track of the number of nonlinear iterations, residual norms, and nonlinear increment norms. Thus, any explicit passes of these values between SolutionStrategy.after_nonlinear_convergence, SolutionStrategy.check_convergence, and NewtonSolver.solve are removed. Conveniently, this fixes iteration_counter lagging behind by one step.

On an additional note, we noticed that the test suite does not cover the case of divergence due to nan values in SolutionStrategy.check_convergence, i.e., lines 547-551 in solution_strategy.py. Wwe only noticed a mistake in our changes due to flake complaining, not the tests. Any opinions on that would be welcome :)

Types of changes

What types of changes does this PR introduce to PorePy?

  • 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:

Not quite sure how to categorize this, as it is only code cleanup, but any custom scripts depending on iteration_counter and check_convergence will break.

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.

…nd nonlinear_increment_norm between solver methods
Copy link
Contributor

@IvarStefansson IvarStefansson left a comment

Choose a reason for hiding this comment

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

Thanks!
Would it make sense to add a test that runs for a fixed number of iterations and than e.g. checks that at the end the iteration count is as expected?


solver_progressbar.update(n=1)
# Ignore line being too long, because we would need an additional
# variable to fix this.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you agree with this, @keileg?

break

if not is_converged:
model.after_nonlinear_failure()

return is_converged, iteration_counter
return is_converged, model.nonlinear_solver_statistics.num_iteration
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it important to return the counter?

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.

Placement of the iteration counter in the Newton solver
2 participants