-
Notifications
You must be signed in to change notification settings - Fork 134
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
Updates to advanced snow physics implementation #449
Changes from 1 commit
0bccb30
2e12190
e4e04d4
d6d8d45
8fe89cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes 141, 267 and 268 would be a problem with other ktherm options and the snow grain tracer active. Since the internal assignments of massice and massliq are wrapped in a ktherm == 2 statement, they would be sent as zeros to thickness_changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first look didn't catch this. Probably the simplest fix is to just pull the assignment statements out of the ktherm == 2 if-else and move to after both the temperature_changes routines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to support advanced snow physics on BL99 thermo? We could abort if that combination is attempted. Or we could initialize massice and massliq according to the BL99 assumptions (use rhos for massice and 0 for massliq?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think just moving it out of the if statement is easiest. It has it's own if snwgrain ... wrapper. I'm certainly not in favor of supporting a massive number of config combinations, but the snow stuff doesn't really care about mushy or BL99. That's not to say I've tested it with BL99... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean line 333ff? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I agree that should be changed. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eclare108213 : line 1294 in icepack_tracers.F90 should also be changed to rhosnew. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This else part of this statement in icepack_snow.F90 is a problem. rhos_cmpn can't be used at all if snow effective density is false otherwise it will overwrite ice area. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the first part of the if statement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Will it work to just remove the else part? This is part of the reason for defining the all of the tracers when tr_snow=T. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, deleting L318 and 319 works |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,7 +142,7 @@ cat >> ${jobfile} << EOFB | |
###SBATCH --mail-user [email protected] | ||
EOFB | ||
|
||
else if (${ICE_MACHINE} =~ badger*) then | ||
else if (${ICE_MACHINE} =~ chicoma*) then | ||
cat >> ${jobfile} << EOFB | ||
#SBATCH -J ${ICE_CASENAME} | ||
#SBATCH -t ${ICE_RUNLENGTH} | ||
|
@@ -152,7 +152,9 @@ cat >> ${jobfile} << EOFB | |
#SBATCH -o slurm%j.out | ||
###SBATCH --mail-type END,FAIL | ||
###SBATCH [email protected] | ||
#SBATCH --qos=standby | ||
#SBATCH --qos=debug | ||
##SBATCH --qos=standard | ||
##SBATCH --qos=standby | ||
EOFB | ||
|
||
else if (${ICE_MACHINE} =~ discover*) then | ||
|
This file was deleted.
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.
@eclare108213 : For the snwredist options, it seems we're only supporting ITDrdg and none. Are bulk, 30percent and ITDsd defunct?
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.
We're supporting ITDrdg, bulk and none. The old 30percent is available - it's set using 'bulk' with a (new) namelist parameter that can be any percentage. ITDsd is defunct.
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.
Good to know. I'll have to look into how the bulk option would work in mpas-seaice. Does 'bulk' use the effective snow density?
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.
I think bulk works the same way as 30percent did before, just generalized the percentage. So it only rearranges the snow without changing the density. And yes the density tracer is still available in that case - it should remain constant, although I haven't looked at that yet after this latest round of changes.
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.
I'm concerned that the way it's implemented, we are requiring the density tracer for the bulk option. Otherwise it will fail. And it doesn't do anything.
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.
It doesn't fail in Icepack (or CICE). Can MPAS-SI not implement it that way? With it, analysis scripts looking for it (for comparison with ITDrdg) won't fail, and its values can be verified to be constant.
Have you tried running this icepack version in your E3SM tests? I think it's coded so that if the driver doesn't have the tracer, icepack will still handle it gracefully. But I'm not sure about that. If it doesn't work, then I can change it.
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.
I haven't tested (recently) snwredist = bulk and snow effective density off, but I will. But tracers are passed to icepack in the tracer array which in mpas is only made up of active tracers. The index for nonactive tracers is set to 0. So in something like
do k = nt_rhos, nt_rhos+nslyr-1
trcrn(k,n) = max(trcrn(k,n)-rhosmin, c0)
I suspect F90 won't like that nt_rhos = 0, and the rest of the code will overwrite something valuable.
I could be wrong. I've almost got your changes implemented. I'll try running with different options.