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

Line search #1208

Closed
wants to merge 8 commits into from
Closed

Line search #1208

wants to merge 8 commits into from

Conversation

IvarStefansson
Copy link
Contributor

Proposed changes

This PR adds functionality for a line search to help convergence of multiphysics problems with fracture deformation.

Some of the functionality could be tested. To be discussed with reviewers. Same goes for naming and placement of code.

Types of changes

What types of changes does this PR introduce to PorePy?
Put an x in the boxes that apply.

  • 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

Put an x in the boxes that apply or explain briefly why the box is not relevant.

  • 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

@IvarStefansson IvarStefansson requested review from jwboth and removed request for OmarDuran August 7, 2024 16:11
@IvarStefansson IvarStefansson self-assigned this Aug 7, 2024
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
tutorials/solution_strategies.ipynb Show resolved Hide resolved
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.

Partial review; I made to the point where the line search got technical. TBC.

src/porepy/models/solution_strategy.py Outdated Show resolved Hide resolved
src/porepy/models/solution_strategy.py Outdated Show resolved Hide resolved
ind = max_arg_1 - max_arg_2
if self.params.get("adaptive_indicator_scaling", False):
# Scale adaptively based on the characteristic fracture traction estimate.
# Base on all fracture subdomains.
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the same scaling is applied uniformly for all the subdomains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that it is computed based on the values from all fractures, not just the present one. This "feels" more robust. Let's discuss if necessary.

src/porepy/models/solution_strategy.py Outdated Show resolved Hide resolved
src/porepy/models/solution_strategy.py Show resolved Hide resolved
src/porepy/numerics/nonlinear/line_search.py Show resolved Hide resolved
src/porepy/numerics/nonlinear/line_search.py Outdated Show resolved Hide resolved
src/porepy/numerics/nonlinear/line_search.py Outdated Show resolved Hide resolved
tol = 1e-1
f_0 = objective_function(0)
f_1 = objective_function(1)
if f_1 < model.params["nl_convergence_tol_res"] or (f_1 < f_0 / 1e4):
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 correct that the first part here is an absolute criterion, while the second is effectively a relative criterion? How should these parameters relate to the Newton convergence parameters (if at all)? Maybe I'm just confused, but I'm not sure I agree with the comments in the lines below.

Then again, this is just an aid during the NL solution procedure and some pragmatism may be in order here. @jwboth What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. My thinking is that the residual/global line search is very unsophisticated, so I didn't bother to clean up hardcoded criteria etc. I think, however, that we can fairly safely remove the second criterion here. Would that be preferrable for now? Simpler=better, if no justification for more complex code...

src/porepy/numerics/nonlinear/line_search.py Show resolved Hide resolved
Copy link
Contributor Author

@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.

I think I've addressed most comments. Please let me know what you think, @keileg. If we can resolve (almost) everything, I'll close the PR and make a new one into develop.

@IvarStefansson IvarStefansson removed the request for review from jwboth August 9, 2024 09:56
@keileg
Copy link
Contributor

keileg commented Aug 9, 2024

List of unresolved questions/issues:

  1. The need to almost copy equation definitions from the models to the numerics-model interface, here
  2. absolute and relative tolerances in line search

I suggest we take note of these, close and make a new PR.

@IvarStefansson
Copy link
Contributor Author

List of unresolved questions/issues:

  1. The need to almost copy equation definitions from the models to the numerics-model interface, here
  2. absolute and relative tolerances in line search

I suggest we take note of these, close and make a new PR.

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.

2 participants