Added diagnostics for partial derivatives of density#1142
Conversation
| do k=1,nz | ||
| pressure_1d(:) = pressure_1d(:) + 0.5 * h(:,j,k) * GV%H_to_Pa ! Pressure in middle of layer k | ||
| ! To avoid storing more arrays, put drho_dT into Rcv, and drho_dS into work3d | ||
| call calculate_density_derivs(tv%T(:,j,k),tv%S(:,j,k),pressure_1d, & |
There was a problem hiding this comment.
You need to import this function in module specification (L18-19).
(Was this able to build for you?)
There was a problem hiding this comment.
Aha, this was my error. I forgot to push my latest commit after making that change 🤦
There was a problem hiding this comment.
All good 🎉
BTW do you think you are able to sort out the OpenMP error as well? If not, then I can look into it.
There was a problem hiding this comment.
I don't think I have the knowledge to sort that. I'd be happy if you would talk me through it though.
Is it failing because of something in the changes that I've made? Or have I done something wrong in not merging my branch with dev/gfdl correctly in the first place?
There was a problem hiding this comment.
The OpenMP directive preceding the block (line starting with !$OMP) has an error because you have default(none) and haven't specified if work_3d is shared across threads or not.
The directive could be fixed by adding work_3d to the OpenMP directive. But if you did not mean to add it, then it might be simpler to just remove it.
There was a problem hiding this comment.
Not knowing how OpenMP functions, I don't have a sense for whether it is necessary to have work_3d shared across threads. I was just copying the convention of the other density calculations in including this line.
I think, in fact, that the line as present in my PR was copied from a previous version of MOM_diagnostic.F90, as I see that the OpenMP directives for the other density variables (e.g. rhoinsitu) have changed. I wonder if this line should instead just be consistent with that for rhoinsitu? i.e. !$OMP parallel do default(shared) private(pressure_1d)
I will implement that just now and you can tell me if I'm missing something.
There was a problem hiding this comment.
That looks good, and also looks like it passed. (Just waiting on the ARM tests...)
The regression fail is expected since this is a new diagnostic. I'll push this to Gaea and if it passes then I will merge it.
Thanks for fixing that up!
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #1142 +/- ##
============================================
- Coverage 46.08% 45.79% -0.30%
============================================
Files 214 223 +9
Lines 69399 69847 +448
============================================
Hits 31984 31984
- Misses 37415 37863 +448
Continue to review full report at Codecov.
|
|
Gaea regression tests: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/pipelines/10739 |
|
Gaea regression has passed. Parameters will need to be updated. Travis CI testing has also passed, but it's failing to update to GitHub. I've triggered another run but we may need to override this. |
4 participants
In
diagnostics/MOM_diagnostics.F90I added diagnostics for partial derivative of density wrt temperature and salinity. I followed the convention of the density diagnostics. The new diagnostics ardrho_dTanddrho_dS.