Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Icepack Interface Refactor #255

Open
apcraig opened this issue Mar 13, 2019 · 6 comments
Open

Icepack Interface Refactor #255

apcraig opened this issue Mar 13, 2019 · 6 comments

Comments

@apcraig
Copy link
Contributor

apcraig commented Mar 13, 2019

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
Copy link
Contributor

eclare108213 commented Mar 13, 2019

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
Copy link
Contributor Author

apcraig commented Mar 13, 2019

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
Copy link

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
Copy link
Contributor

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

@apcraig
Copy link
Contributor Author

apcraig commented Dec 9, 2019

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
Copy link
Contributor

eclare108213 commented Oct 1, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants