-
Notifications
You must be signed in to change notification settings - Fork 227
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
added diagnostic for internal heat in 3D #981
added diagnostic for internal heat in 3D #981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gmacgilchrist, I added some code style comments, including changes needed to pass the test suite.
The diagnostic itself looks OK but will have a look with one of the other guys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmacgilchrist, I had a couple of comments, but nothing major. Not officially 'reviewing' this PR, but looked at it out of curiosity.
I have a more general inquiry for @Hallberg-NOAA and @adcroft about the approach to geothermal heating done here. I understand that it makes sense to distribute the heat over a finite depth range to avoid just dumping heat into a 'vanished' layer. Why does this routine do the calculation of redistributing layer thickness in isopycnal mode. Couldn't this be simplified to dump the heat into the first non-vanished layer, and then let MOM_entrain_diffusive deal with sorting the densities?
Latest commit (still just for heat, no thickness or temperature diagnostics included yet) does the calculation within the main for-loop, and puts everything behind conditional statements. Not sure if it is done in the best or most efficient way, so I would welcome feedback and criticism. Most of what I've done is based on code in MOM_diabatic_driver.F90. In that module, calculation of the tendency diagnostics are carried out in their own subroutine. Might this be something worth considering here, or is that overkill? |
Hi Graeme, just chatted with Alistair and it looks like I gave you a bit of bad advice regarding the need to conditionally allocate the variables. This declaration is OK (ignoring the character limit):
as long as all of the operations on
This is sufficient to ensure that the memory will never be touched or added to the stack when the diagnostic is not used. Apologies for the confusion here. If you fix this, address the missing indices in |
Thanks @marshallward, this makes sense. Can I just confirm that the procedure for deallocating the diagnostic arrays is done correctly? I just included it into the conditional statement after the diagnostic has been posted. ^ actually forget that... I won't need to allocate or deallocate given what you've just said. |
OK, here's another crack, including diagnostics for temperature and for thickness, and making the allocation implicit when values are assigned. I think that the logic is good, but would appreciate someone double checking this. I was also not sure what to do about naming the diagnostics. Is there a general convention that people stick to? I tried to stick to the conventions of the tendency diagnostics already available (although these are somewhat scattered). This makes them rather verbose though (e.g. internal_heat_heat_tendency)... |
This all looks great, thanks @gmacgilchrist. I will merge this now. (Edit: After testing in the CI.) |
GitLab regression test: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/pipelines/8708 |
Regression tests have passed, merging now. |
Previous diagnostic for heat tendency due to geothermal heating was only a 2D field, with no information on the model layer into which the heat is added, preventing closure of cell-wise heat budget. Created a new diagnostic in MOM_geothermal.F90, called
internal_heat_tend_3d
that keeps track of the change in the 3d heat field heat before and after geothermal heating is applied.