CICE-Consortium / Icepack

Development repository for sea-ice column physics
Other
25 stars 131 forks source link

Update icepack_step_therm2, make fsd arguments optional #440

Closed apcraig closed 1 year ago

apcraig commented 1 year ago

PR checklist

Also update interface documentation.

apcraig commented 1 year ago

@eclare108213, that's a good question. The optional argument attribute is not required in lower level routines. You may not be able to check whether that argument is present anymore in the lower level routines, but as long as you DON'T use an optional argument that was not passed above, the code should compile and run fine whether the optional attribute is on the lower level routines or not. As we know, if you don't pass an optional argument then try to use it, whether it's declared optional or not in the lower level routines, you should get a runtime error. The lower level optional arguments don't change that. If you prefer, I can remove the optional arguments on the lower level routines for consistency. I don't feel strongly that it has to be one way or the other, but we can define a standard if you like. There may be times where that optional argument attribute has to be set in lower level routines, but hopefully that's not the case most of the time.

eclare108213 commented 1 year ago

I'd prefer that we do it consistently -- otherwise it's too confusing when implementing new arguments (or fixing old code). There was an issue a few months back, maybe with a snow variable? and now I worry that the code isn't as robust with all the optional args. Does the optargs unit test provide a template for how this is supposed to work? If so, maybe that could be pointed out in the docs, or some pseudo-code provided there. I searched for 'optional' and didn't find anything like that.

apcraig commented 1 year ago

I updated the lower level optional arguments and retested everything, still working fine.

Section 5.2 of the Icepack users guide talks about optional arguments. It's relatively current, but let me update that a bit.

apcraig commented 1 year ago

https://cice-consortium-icepack--440.org.readthedocs.build/en/440/developer_guide/dg_col_phys.html

lettie-roach commented 1 year ago

This looks fine to me. Did your test to check BFB-ness have tr_fsd=True?

apcraig commented 1 year ago

I ran a full test suite, that includes tests with tr_fsd=true, all results are bit-for-bit.