Skip to content

Ensure correct summary output for iteration counts.#6720

Merged
bska merged 1 commit into
OPM:masterfrom
atgeirr:fix-newton-iter-summary
Jan 14, 2026
Merged

Ensure correct summary output for iteration counts.#6720
bska merged 1 commit into
OPM:masterfrom
atgeirr:fix-newton-iter-summary

Conversation

@atgeirr
Copy link
Copy Markdown
Member

@atgeirr atgeirr commented Jan 9, 2026

Summary NEWTON array (and similar) is off by one, and wrong after a timestep cut.

@atgeirr atgeirr added the manual:bugfix This PR is a bug fix and should be noted in the manual label Jan 9, 2026
@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 9, 2026

jenkins build this please

akva2
akva2 reviewed Jan 9, 2026
View reviewed changes
Comment thread opm/simulators/timestepping/AdaptiveTimeStepping_impl.hpp Outdated
full_report += substep_report;
problem.setSimulationReport(full_report);
// This also runs the evalSummary() and action updates.
problem.endStepApplyAction();
Copy link
Copy Markdown
Member

@akva2 akva2 Jan 9, 2026

Choose a reason for hiding this comment

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

Not sure it is relevant right now, but FlowProblemComp does not have this method.

Copy link
Copy Markdown
Member Author

@atgeirr atgeirr Jan 9, 2026

Choose a reason for hiding this comment

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

I see it is all done in endTimeStep(). If we do the same modification there it should become correct.

The test failures are all expected (only modifying the data I wanted to) except the last one (flowexp_blackoil_SPE1CASE2) which I will need to investigate.

@atgeirr atgeirr force-pushed the fix-newton-iter-summary branch from f2db002 to 0a93c74 Compare January 12, 2026 15:22
@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 12, 2026

jenkins build this please

@atgeirr atgeirr force-pushed the fix-newton-iter-summary branch from 0a93c74 to e4b4bfc Compare January 13, 2026 09:28
@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 13, 2026

jenkins build this please

1 similar comment
@akva2
Copy link
Copy Markdown
Member

akva2 commented Jan 13, 2026

jenkins build this please

@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 13, 2026

All failures are now in the expected fields and should be corrections compared to the reference.

@atgeirr atgeirr requested a review from bska January 13, 2026 12:50
@bska
Copy link
Copy Markdown
Member

bska commented Jan 13, 2026

All failures are now in the expected fields and should be corrections compared to the reference.

Okay, but just to simplify the review I'll create a "failure report" for visual inspection.

@bska
Copy link
Copy Markdown
Member

bska commented Jan 13, 2026

jenkins build this failure_report please

@bska
Copy link
Copy Markdown
Member

bska commented Jan 13, 2026

All failures are now in the expected fields and should be corrections compared to the reference.

Okay, but just to simplify the review I'll create a "failure report" for visual inspection.

I've looked at the failure report now and I agree that the effect of this work is as promised. I'll review the implementation now.

@bska
Copy link
Copy Markdown
Member

bska commented Jan 13, 2026

I'd prefer if we did not have to introduce the setCall..() hook. To me, it seems that the fundamental problem is that NonlinearSolver::step() calls Model::afterStep(), and that BlackoilModel<>::afterStep() invokes Problem::endTimeStep() while measuring its runtime.

Could we achieve a similar effect to this PR if we instead made BlackoilModel<>::afterStep() do nothing–i.e., just return an empty report structure to satisfy the interface requirements–and then effectively move the "real" call to Problem::endTimeStep() into the location that's modified in this PR? That way, I think, we could avoid having to introduce the callApply...() hook.

@atgeirr atgeirr force-pushed the fix-newton-iter-summary branch from e4b4bfc to 3c53df1 Compare January 14, 2026 14:48
@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 14, 2026

Refactored as requested. A followup should be to remove the prepareStep() method as well, replacing it with calls to beginTimeStep() outside the NonlinearSolver::step() method, but that is not required now I think.

@atgeirr
Copy link
Copy Markdown
Member Author

atgeirr commented Jan 14, 2026

jenkins build this please

@bska
Copy link
Copy Markdown
Member

bska commented Jan 14, 2026

jenkins build this failure_report please

bska
bska approved these changes Jan 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Refactored as requested.

Thanks a lot for the updates. This looks good to me now and I've verified that it still does what it's supposed to do. I'll update the reference solutions and merge this now.

A followup should be to remove the prepareStep() method as well, replacing it with calls to beginTimeStep() outside the NonlinearSolver::step() method, but that is not required now I think

I think that's a good plan for follow-up work.

@bska
Copy link
Copy Markdown
Member

bska commented Jan 14, 2026

jenkins build this update_data please

@jenkins4opm jenkins4opm mentioned this pull request Jan 14, 2026
Merged
jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request Jan 14, 2026
Automatic Reference Data Update for PR OPM/opm-simulators#6720
743b161 
Reason: PR OPM/opm-simulators#6720

opm-common     = 791a4b71bed5b2f848918af1e994dcc030b94c0a
opm-grid       = c343380b8b3741038959c4719e3ab66034c7e436
opm-simulators = 673ce2ae63bb3670b95f27e7b88a6812b29a0787

### Changed Tests ###

  * gasoil_precsalt
  * udt-1d-03
  * spe1case2_krnum
  * norne_reperf
  * base_model_1
  * udt-1d-01b
  * network-01_standard
  * udt-1d-02
  * udt-1d-01
  * msw_model_1
  * faults_model_1
  * fpr_nonhc
@bska
Copy link
Copy Markdown
Member

bska commented Jan 14, 2026

jenkins build this opm-tests=1464 please

bska added a commit to OPM/opm-tests that referenced this pull request Jan 14, 2026
@bska
Merge pull request #1464 from jenkins4opm/update_opm-simulators_6720
55e4ee9 
Automatic Reference Data Update for PR OPM/opm-simulators#6720
@bska
Copy link
Copy Markdown
Member

bska commented Jan 14, 2026

Refactored as requested.

Thanks a lot for the updates. This looks good to me now and I've verified that it still does what it's supposed to do. I'll update the reference solutions and merge this now.

The new reference solutions have been installed on the CI system so I'll merge this into master. Thanks a lot for identifying and fixing this problem.

@bska bska merged commit a34d561 into OPM:master Jan 14, 2026
2 checks passed
@atgeirr atgeirr deleted the fix-newton-iter-summary branch January 16, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akva2 akva2 akva2 left review comments

@bska bska bska approved these changes

Assignees

No one assigned

Labels

manual:bugfix This PR is a bug fix and should be noted in the manual

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@atgeirr @akva2 @bska