CICE-Consortium / Icepack

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

Icepack Interface Refactor #255

Closed apcraig closed 3 weeks ago

apcraig commented 5 years ago

A number of concerns associated with the icepack interfaces have been raised and an interface refactor is being considered. A google doc has been created to provide a single location to aggregate input and to facilitate creation of a design document. I hope the google doc will lead to a design and implementation plan should we decide to move forward.

https://docs.google.com/document/d/1jgNia1OddbfXW99qFw7r_0d8X_ikrB2oXXF--juZC3c

This should be publicly viewable and I will give most of the Consortium Team write access. Feel free to edit the google doc directly, email me with comments/thoughts separately, or even add comments under this github issue. I will try to keep the google doc fresh.

eclare108213 commented 5 years ago

Thanks @apcraig For the two proposed approaches (keywords and data types) for mitigating issues with long hardwired subroutine argument lists, I would like to know what experience modeling centers/models have with them. What is the best way to evaluate these options, short of implementing both and comparing? Are there other potential solutions? @philipwjones do you have suggestions for this?

apcraig commented 5 years ago

Thanks @eclare108213. Let me also just clarify that the current google doc is something I put together in 30 minutes. We need to flush out other issues, other requirements, and other design ideas. The current document is far from comprehensive, but hopefully serves as a starting point. I hope we will add significant content to the document over the next days/weeks and reorganize the document as needed to meet our needs.

philipwjones commented 5 years ago

Sorry to be slow in responding. A few comments on the plan so far. Despite the length of argument lists, explicit arguments with clear in/out intent is still going to be preferred for the ability to determine dependencies and for new programming models that infer data flow or create a DAG from those dependencies. Long argument lists can be alleviated with the use of user-defined types or structs. But I would not recommend a large pbuf-like type since we'd still like some granularity in the dependency analysis to determine where some process/task parallelism can be exploited. Also, we'd like very shallow types for both compiler efficiency (avoiding need for deep copies) and because it's easier to extract data without chasing pointers down a deep structure.

Regarding keywords and optional args - this is a Fortran feature (and a nice one at that!) that can be a risky approach as we continue down the road toward a mixed-language environments and ongoing pressure to abandon Fortran. Something to consider since Icepack needs to be adopted by newer C-based ice codes in the pipeline and long-term support for Fortran continues to decline.

eclare108213 commented 5 years ago

See description of interface changes needed by GFDL here: https://github.com/MichaelWinton/SIS2/blob/melt_pond/docs/icepack.md

apcraig commented 4 years ago

Just to note, some refactoring was done as part of #282, #285. Keyword pairs were added to arguments in the Icepack driver and CICE. Several interfaces were renamed, a few interfaces were made public thru icepack_intfc, and the icepack_tracer init/query/write interfaces were rearranged a bit. In addition, some argument names changed, some arguments were dropped, and in one subroutine, the full tracer array was split into arrays for tracers.

This effort is still ongoing.

eclare108213 commented 1 year ago

The names of some routines are no longer representative of their contents (portions of them having been moved out into other subroutines), and so we could rename them, e.g. icepack_init_thermo becomes icepack_init_salinity icepack_init_trcr becomes icepack_init_enthalpy Could we keep both versions of the subroutine names in the interface, both pointing to the new subroutine name, for backward compatibility?
Low priority.