-
Notifications
You must be signed in to change notification settings - Fork 152
Fixes for sitemptop, sitempbot, and sitempsnic. #1054
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ae5b8fd
Fixes for sitemptop, sitempbot, and sitempsnic.
dabail10 0dfcba1
Change the averaging for the sitemp variables
dabail10 13de839
Fix comment
dabail10 1bd92f4
Address issues with averaging when ice present. Also, change cell_met…
dabail10 069d89c
Cleanup the code a bit more
dabail10 fea53d7
Fix indent
dabail10 7552bd1
Update icepack with Tsnice fix
dabail10 58de585
Fix syntax error in ice_history.F90
dabail10 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
is it useful to add a comment here,
e.g. sitempsnic is intensive, and Tsnice is already weighted by aice in icepack
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.
Why is Tsnice treated differently from the other surface temperatures? This is confusing. Could the multiplication by aice be done here instead?
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 guess I am not sure what you are saying. Tsnice is computed in step_therm1 and aggregated using aicen_init. Whereas Tsfc is advected and changed after the dynamics. Also, trcr(:,:,nt_Tsfc) is divided by sum(aicen(:)).
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.
In this section of code, the Tsfc tracer and Tbot are multiplied by aice, but Tsnice is not. I think it would be less confusing if all three were sent out of Icepack in the same form. I know they are computed differently in Icepack, but shouldn't the end result that is sent to the driver be something like "a surface temperature (top, ice-snow interface, or bottom) that is averaged only over the ice that's present" for all three? At the moment it looks like Tsfc and Tbot are averaged over the ice and Tsnice is averaged over the grid cell when they are sent out of Icepack.
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 feel like I am going around in circles on this one. The idea was that we did not want Tsnice as:
and then multiply by aice again in the history averaging. So, Icepack is just sending:
thus we do not need to mutilply again by aice when accumulating in ice_history.F90. However Tsfc and Tbot do require multiplication by aice.
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.
Then Tsfc and Tbot are the ones that ought to be fixed. Let's plan to clean this up when we fix the general aice and aice_init issues rather than in this PR. Thanks for the clarification.
There are a lot of "will do so-and-so in a later PR" in the comments here -- need to keep track of them in an issue (new or updates to an existing one)...