First part of adding Tf. #12
Conversation
|
Ready to merge? |
apcraig
merged commit 19d61e6
into
E3SM-Project:cice-consortium/E3SM-icepack-initial-integration
|
@dabail10 does this PR take care of items 14-20 at https://acme-climate.atlassian.net/wiki/spaces/ICE/pages/3420979263/MPAS+hashes+with+changes+to+column+package? |
|
Not completely. I am still working on the algae piece. |
|
Okay, that's item 17. What about the new functions in 18? This hash: MPAS-Dev/MPAS-Model@f5924be3 |
|
@dabail10, did you do any testing in CICE? I'm trying to test the latest Icepack in apcraig:CICE/E3SMIcepackDEV and interfaces have changes, Tf is now required in some of the interfaces, and I'll have to do some study to better understand the needed changes. It's fine that interfaces changed, but we probably need to be able to update CICE fairly quickly in response. I wonder if it wouldn't have been better to fix all the Tf stuff in a single PR? Ultimately, we'll also have new namelist (for Tfrzocn) and maybe other changes, it might have been nice to do all of that at once in CICE. The bit-for-bit and non-bit-for-bit changes can be done as separate commits in the development branch, and the PR could be done at the end. Maybe a lesson for next time? Right now, we either need to update CICE to meet the latest Icepack implementation or not update Icepack in CICE and wait. What's the time frame for getting the rest of the Tf changes into Icepack? Just trying to decide whether to wait or do an update now and again later. |
|
I did not test in CICE. I was kind of waiting on the BGC related change. Also, I admit I am confused at this point by the E3SM branch. |
|
The BGC stuff is probably not going to be implemented for a little while. I'm happy to help with any questions about the E3SM Icepack/CICE development and testing. Over the weekend I did a bunch of testing and verified that the CICE Consortium main (#422117fc5d) is bit-for-bit with apcraig:CICE/E3SMIcepackDEV #968948928a. I agree the process is confusing and challenging, so it's important to be thoughtful. For now, I think the main question is what to do with the partly finished Tf implementation. Another option would be to revert the PR and then wait until the Tf is finished to PR it. The main issues we need to be careful about are
Forgetting about the Consortium versions for a moment, the "main" branches for the E3SM Icepack development are E3SM-Project:Icepack/cice-consortium/E3SM-icepack-initial-integration those are the two branches that need to stay synced up and where we need to be tracking bit-for-bit results. I don't think Icepack changes always have to be tested in CICE if they're bit-for-bit in Icepack testing and unlikely to change answers in CICE. The issue with the Tf PR is the interface changes that we need to sort out. |
|
That makes sense. Unfortunately I am heading out of town tomorrow for a week, so I won't get a chance to make the CICE changes necessary. |
|
Sorry I wasn't thinking about interface changes when reviewing this. Those do need to be backwards-compatible in CICE, and it sounds like they aren't. It's probably wise to revert this merge and continue working on the branch. I'd also like to double check that it has all of the functionality that E3SM is expecting. I'm planning to work on the aerosols code next, which might take some time to complete. |
|
Do we want to make Tf optional in the interfaces? I know it comes in through icepack_step_therm1 or therm2 ... however, it needs to be added to icepack_stepdyn_ridge I believe? Then it would need to be added to the BGC interfaces when that comes in. |
|
I think it makes sense to revert the PR at this point if it's going to be a while to finish the task. I could also try to make progress in the next day to get this more complete and get CICE updated. I know we need to add Tfrzocn to the namelist and pass it into the icepack parameters. That's easy enough to do. Are there other things? I don't know if it makes sense to make Tf optional in the interface. It seems like a pretty critical gridcell variable. I worry if it were optional, then users could easily forget to pass it and end up with bad science. If we did make it optional, I think we then need to have a local logical like (Tf_passed) and then the code should abort if Tf_passed=false and it needs Tf. If Tf is always needed in Icepack, then there is no reason to make it optional. |
|
Tf isn't passed into many of those routines now (i.e. consortium main), and so they should still be able to work without it here when using appropriate namelist choices. If Tf were optional, then an "if present" check could be made at the top level that also checks the namelist configuration for consistency? If not, then abort. And, if Tf is needed, maybe the local gridcell value could be stored in an Icepack parameter on each timestep, which is used (or queried) when needed, rather than passing down through the calling tree? Just some ideas. |
|
I thought about the idea of setting Tf via icepack_parameters, but I don't think that's the intent. That's for more like static scalar data. Tf is definitely something that could/will vary by time and gridpoint, so I think it should be passed into the Icepack runtime interfaces. Are there configurations where Tf is not needed? Or are you suggesting it's not needed if Tfrz_option = minus1p8 for instance because it's easy enough to determine Tf internallly in Icepack in that case? |
Clean up icepack_shortwave
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Added Tf to all the subroutine calls and used in place of Tocnfrz.
dabail10 (D. Bailey)
@eclare108213 @apcraig
Compared two single cases with and without the change.
This is the first step in this change. The next will change answers for the BGC. Note this might also change answers in CICE at least in the history files.