Skip to content

Fix indexing issue for output monitoring#3389

Open
Steven-Roberts wants to merge 1 commit into
boutproject:nextfrom
Steven-Roberts:monitor-bug-fix
Open

Fix indexing issue for output monitoring#3389
Steven-Roberts wants to merge 1 commit into
boutproject:nextfrom
Steven-Roberts:monitor-bug-fix

Conversation

@Steven-Roberts

Copy link
Copy Markdown
Contributor

Output monitoring functions would get called twice in a row with iter = 0 (unless dump_on_restart = false) and would never reach iteration NOUT which was used as a check for cleanup:

if (iter == NOUT || abort) {

This PR fixes solver iterations to be 1-indexed.

@dschwoerer dschwoerer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this break the output? I think there are somewhere some tricky details, especially with smaller / slower output monitors, that make things a bit non-intuitive ...

@dschwoerer

Copy link
Copy Markdown
Contributor

Why not change the check to NOUT - 1 instead of changing 19 files?

@ZedThree

Copy link
Copy Markdown
Member

Does this break the output? I think there are somewhere some tricky details, especially with smaller / slower output monitors, that make things a bit non-intuitive ...

I'm also a bit wary about touching this stuff because I recall how fiddly the details are, but we do have pretty good tests on calling multiple monitors with different frequencies (SolverTest.AddMonitorCheckFrequencies for example), and that still seems to pass, so I'm in favour of merging this. It does also seem to make the logic easier to understand, even if do end up with some 1-indexing loops!

@dschwoerer

Copy link
Copy Markdown
Contributor

The output, at least for non restarted simulations also seem to match, so might be good.

@Steven-Roberts could you please merge Steven-Roberts#1 to fix the formatting?
The CI is failing because it is from a fork, otherwise it would have found those issues.

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