shortwave dEdd_snicar_ad setup
apcraig opened this issue · 10 comments
This affects both Icepack and CICE. Several issues,
- should the default "dEdd" shortwave setting be updated to dEdd_snicar_ad from dEdd?
- the USER_SNICARHC cpp if turned OFF by default because it takes extra time to compile and is not used very often. This is in Icepack. Should that code actually be ON by default with an option to turn it off with a cpp?
See also #881 which is not expected to be merged but has some discussion.
I spent some time trying to understand why icepack_shortwave_data takes so long to compile when USE_SNICARHC is enabled. I tested on cheyenne, both intel and gnu. CICE compile time increases from 2 to 6 minutes for intel and 1 to 3 minutes for gnu.
I did many things including
- eliminate the temporary arrays in icepack_shortwave_init, set the module data directly
- change the module data from pointers to allocatable+target and then to allocatable
- get rid of the pointers to the table data in icepack_shortwave, subroutine compute_dEdd_5bd and use the module data directly
- tested several ways of setting the table data,
ssp_sasymmdr(1:5 ,1) = (/ 0.883812826619966_dbl_kind, 0.883341999504413_dbl_kind, 0.882286247177836_dbl_kind, 0.883104831607199_dbl_kind, 0.898615034787299_dbl_kind /)
ssp_sasymmdr(1 ,1) = 0.883812826619966_dbl_kind
ssp_sasymmdr(2 ,1) = 0.883341999504413_dbl_kind
ssp_sasymmdr(3 ,1) = 0.882286247177836_dbl_kind
ssp_sasymmdr(4 ,1) = 0.883104831607199_dbl_kind
ssp_sasymmdr(5 ,1) = 0.898615034787299_dbl_kind
ssp_sasymmdr(1 ,1) = 0.8838128_dbl_kind
ssp_sasymmdr(2 ,1) = 0.8833419_dbl_kind
ssp_sasymmdr(3 ,1) = 0.8822862_dbl_kind
ssp_sasymmdr(4 ,1) = 0.8831048_dbl_kind
ssp_sasymmdr(5 ,1) = 0.8986150_dbl_kind
ssp_sasymmdr(1 ,1) = c0 ! 0.8838128_dbl_kind
ssp_sasymmdr(2 ,1) = c0 ! 0.8833419_dbl_kind
ssp_sasymmdr(3 ,1) = c0 ! 0.8822862_dbl_kind
ssp_sasymmdr(4 ,1) = c0 ! 0.8831048_dbl_kind
ssp_sasymmdr(5 ,1) = c0 ! 0.8986150_dbl_kind
ssp_sasymmdr(:,:) = c0
The only thing that helped was the final option above (ssp_sasymmdr(:,:) = c0) which is not an option, but suggests the compile time is not affected by how memory is allocated by the compiler, not the pointers or copies, and not precision of the data. It seems to simply be the number of lines in the file. That suggests that about the only solution for the compile time is to go back to reading a file. If we did that, we would NOT use the old file (where we got this table data to start with), but we would modify it to be more complete and self-describing. It would have to include the values of the arrays of the data among other things. This would speed up compile, but would probably slow down the model runs a bit (maybe just seconds but still....). We would also have to create this new file and place it into the inputdata area. I think the same file would be used for all resolutions.
Also, a problem with setting this table data via a file is that it would have to be read by the "driver" code, either Icepack driver, CICE, or MPASSI for example, and then passed into Icepack (we'd have to extend the parameter interfaces again). In some ways, that's not ideal. I guess another option would be to implement a "file reader" in Icepack as an option. That would require adding some netcdf code to Icepack and a CPP to turn it off. If we want full flexibility, we could have CPPs that enable the hardcoded table, provide the ability for Icepack to read the table from a file (via a subroutine call into Icepack), and/or have the table passed into Icepack from driver code. Having just the hardcoded table is much more straight-forward though and meets the current needs (except for the compile time).
Beginning to think we should stick with the table and just deal with the compile time.
I sort of agree. With an ability to turn on/off the compilation of the table. The question then is whether the default should be compile on or compile off (current default). And are we going to change the default dEdd shortwave setting to dEdd_snicar_ad? If the default shortwave setting changes, the table HAS to be ON by default and we'll pay for that compile nearly all the time.
I agree. We should not turn snicar on as a default until more of our users have had a chance to try it out. E3SM has been using it for years, but that doesn't mean we should recommend it for everyone. And if it's not the default, then the default for the table would also be to not compile it. Can (or do) we tie the compilation to whether snicar is on?
I suspect we will pretty quickly move to dEdd_snicar_ad as the default. Thanks for doing all this work! My personal feeling is to get rid of the CPP and always live with the compile time.
I'm happy to do whatever is best for the community.
In our scripts and testing, if a user picks the "snicar" option that sets shortwave="dEdd_snicar_ad", the USE_SNICARHC is also turned on. So that all works fine. What doesn't work fine is if a user creates a case then manually changes shortwave to "dEdd_snicar_ad". USE_SNICARHC is not turned on by default, the code will compile, but then there will be a runtime error that says "ERROR: USE_SNICARHC CPP required". So that seems to be pretty reasonable overall. All the user has to do is go into cice.settings (or icepack.settings) and set ICE_SNICARHC to true and recompile. Ok, maybe that part is not so clear, but it's an Icepack error and Icepack doesn't know if it's run with the icepack driver, CICE, or something else. So there is a subtle issue about how to tell the user what to do. It's clear what to do if running with Icepack or CICE, Icepack has no idea what to do if the user is running in E3SM or elsewhere.
For CESM, I think the CICE compilation is not used. So @dabail10, you can set USE_SNICARHC all the time in the CESM CICE compile and then use dEdd_snicar_ad as much as you want. I think E3SM does this as well, it sets USE_SNICARHC in it's compile of Icepack. The standalone Icepack and CICE maybe should behave as suggested by @eclare108213 for now? Any maybe later after additional use, we'll change the setup in Icepack/CICE? Again, I don't feel strongly one way or another. I think the only thing we should decide NOW is the name of the CPP. It's currently USE_SNICARHC which suggests the CPP turns on the table. We could also have a CPP called NO_SNICARHC (or similar) which would suggest the CPP turns off the table. In either case, we can have CESM, E3SM, Icepack, and CICE behave the same as now and however each wants. I think once we pick a CPP, we should keep it for the long term though. You can see the current CPPs here, https://cice-consortium-cice.readthedocs.io/en/main/user_guide/ug_case_settings.html#table-of-c-preprocessor-cpp-macros, in case you want to get a sense of how they look now.
Thinking a bit, in some ways, I think NO_SNICARHC is a better CPP to have. That means E3SM and CESM don't have to do anything with a CPP and the snicar table will be available. Same with Icepack and CICE. In Icepack and CICE, we would have NO_SNICARHC turned on by default and off only when needed for testing (behaves same as now). NOW is the time to make this change, before it's out in the community. This will not affect E3SM (USE_SNICARHC will not do anything, but the table will be ON by default). E3SM should just remove USE_SNICARHC at some point so it's clean. CESM has not updated to the latest CICE yet (I assume), so that's OK. I can update Icepack and CICE so the new CPP works and the code behaves the same as now. Thoughts?
That sounds like a reasonable solution. I'm okay just leaving it as it is, too.
OK. I think NO_SNICARHC is better. It means the table is ON if NO CPPs are set which is probably better for users (except for compile time). And the transition to NO CPP is cleaner if we think we'll ultimately compile with the table all the time or even if we go to a file or other method for setting the table. I'll start to work on that in the next day or so and we can still change our mind.