CICE addition of fswthru by components. #479
Conversation
doc/source/cice_index.rst
Outdated
| @@ -252,6 +252,10 @@ either Celsius or Kelvin units). | |||
| "fswint", "shortwave absorbed in ice interior", "W/m\ :math:`^2`" | |||
| "fswpenl", "shortwave penetrating through ice layers", "W/m\ :math:`^2`" | |||
| "fswthru", "shortwave penetrating to ocean", "W/m\ :math:`^2`" | |||
| "fswthru_vdr", "vis dir shortwave penetrating to ocean", "W/m\ :math:`^2`" | |||
There was a problem hiding this comment.
I think for these we should spell out these shortened versions just for clarity. I should also make sure this happens in the Icepack documentation.
vis dir = visible direct, vis dif = visible diffuse, etc. These are obvious to us, but I don't think they are necessarily obvious to others.
There was a problem hiding this comment.
Part of the reason I used the abbreviations here is so that the table did not run over. Here is the icepack table:
https://cice-consortium-icepack.readthedocs.io/en/master/icepack_index.html
I can add the full names if people feel strongly about this.
There was a problem hiding this comment.
The longer spelled out forcing would be more consistent with the albedo components and I think more clear:
alndf | albedo: near IR, diffuse
alndr | albedo: near IR, direct
alvdf | albedo: visible, diffuse
alvdr | albedo: visible, direct
If they run over it should just wrap (see dardg1(n)dt).
There was a problem hiding this comment.
Ok. Made the suggested change to CICE. I have another Icepack PR coming in the next couple days where I will fix it there.
|
Looks like this is ready to merge. I can do that I guess. |
|
Can I hit Squash and Merge? |
|
Hi @dabail10 ! I'm working on compiling CICE within NEMO and noticed you made changes only to the standalone driver in this PR, and not to the other drivers... it's unclear to me if this is an oversight, or do we only update the standalone driver ? it's unclear to me if we have a Consortium policy about that... @apcraig |
|
We do not have a clear policy. Looking back, the changes with this PR were in the standalone driver and the NUOPC/CMEPS cap. It also looks like these new options were not added as optional arguments, so that means the other caps will break. I think we do need to be aware of this issue. I generally try to change all the caps when I'm bringing a change into one. But we have not asked that generally. The problem is that it's not possible to test all of them either. I think we should create a policy where if an interface changes that affects caps, it should be updated in all caps. In this case, it would have been easy and relatively safe to do even without testing. There may be cases where it's not so easy, but we should all try to do that. I can try to create a PR where I update the other caps consistent with this PR. Or @phil-blain, if you are working on it for your cap, maybe you could also include updates to other caps at the same time? |
|
I thought I did make these optional in the interfaces? I have to go back. However, as Tony says, I can't really test all the caps. I did the standalone driver and the CESM/NUOPC cap. |
|
My changes won't be ready for a PR anytime soon, so feel free to do it Tony. |
|
@dabail10, I think the optional part was in the icepack interfaces, not in the interfaces used in the driver/caps. So we probably just need to add the 4 extra arguments to all calls of scale_fluxes. @phil-blain, I'll try to create a PR this week. |
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
This completes the fswthru changes by component already added to icepack.
dabail10 (D. Bailey)
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_mach_forks#cheyenne