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

[ci optional] Implemented Sunny Wong rotation brunt change #553

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rjfarmer
Copy link
Collaborator

People seemed happy to merge this in #372 so let's check to see if the test suite passes and then merge it.

Closes #372

@rjfarmer rjfarmer requested review from orlox and evbauer June 16, 2023 11:18
@rjfarmer
Copy link
Collaborator Author

@evbauer
Copy link
Member

evbauer commented Jun 16, 2023

Hmmm, looks like we have another instance of a photo checksum failure without an fpe: https://testhub.mesastar.org/brunt_in_rotation/commits/69b4927/test_cases/star/ppisn

I suspect there's something non-trivial going on related to the state of brunt-related quantities.

@evbauer
Copy link
Member

evbauer commented Jun 16, 2023

Actually, it looks like this change is definitely not equivalent to the previous way of doing things. We have a line specifically skipping updates to s% brunt_N2 during solver iterations:

if (.not. skip_brunt) then ! skip_brunt during solver iterations
if (dbg) write(*,*) 'call do_brunt_N2'
call do_brunt_N2(s, nzlo, nzhi, ierr)

skip_brunt = .true., &

So the quantities we're trying to take advantage of here are actually going to become stale during solver iterations. I'll push an experimental commit getting rid of that check to see what testing looks like, but I'd definitely like some input from @orlox before proceeding with a merge.

@evbauer
Copy link
Member

evbauer commented Jun 16, 2023

Actually, I'm realizing that was probably a red herring, as we don't update mixing info during solver iterations anyway, so it shouldn't matter if the brunt is getting updated during the iterations here. I'll revert that.

The photo checksum failure does seem to be telling us something important, and it's happening quite consistently, so we'll have to track that down.

@rjfarmer
Copy link
Collaborator Author

If you diff the output of two runs it looks like there are differences immediately after the restart. So I'm guessing there is an issue with how the Brunt is computed at the start of a run that differs from how the old form worked.

Also, https://testhub.mesastar.org/brunt_in_rotation/commits/e03088c/test_cases/star/ppisn shows it's not a parallelisation issue.

Time smoothing needs mstar_old but after a restart we have s% generations=0
so no _old values.
We should be using nzlo:nzhi (though that its almost allways 1:s%nz) anyway.

Also fill local allocatable arrays with nans to look for fpe's
@evbauer
Copy link
Member

evbauer commented Aug 18, 2023

@orlox and I discussed working on this some more today. Not clear on what our timeline will be yet, but I think we at least see a path forward that involves mostly trying to work through all the fpe's we can squash related to rotation and mixing.

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.

brunt in rotation
4 participants