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

Predefined dynamicmask #350

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

bena-nasa
Copy link

Creating PR for discussion of predefined dynamic masking as discussed in last GMAO/ESMF meeting

Copy link
Member

@theurich theurich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking through the file changes, I see one minor whitespace change that should be reverted. Other than that, I do have more high level design questions... which I will write up as a general comment in the conversation of the PR.

@@ -890,7 +890,7 @@ subroutine f_esmf_regrid(srcField, dstField, routehandle, zeroregion, zrpresent,

integer :: localrc
type(ESMF_RouteHandle) :: l_routehandle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whitespace change should be reverted.

@theurich theurich requested a review from oehmke February 17, 2025 23:01
@theurich
Copy link
Member

@bena-nasa @oehmke @tclune - I reviewed this PR, and I have a very high level design question. Basically I am wondering why optional arg of type ESMF_PredefinedDynamicMask enters the methods that handle dynamic masking, i.e. ESMF_ArraySMM() and ESMF_FieldRegrid(). I would have naively thought that the ESMF_PredefinedDynamicMask class is used to generate an ESMF_DynamicMask object, which then can be passed into the unmodified existing methods mentioned above as usual. Maybe I am missing something about the actual use case, and why it necessitates the proposed, more integrated changes. I am sorry if this design aspect has already been discussed and I missed it.

@bena-nasa
Copy link
Author

@bena-nasa @oehmke @tclune - I reviewed this PR, and I have a very high level design question. Basically I am wondering why optional arg of type ESMF_PredefinedDynamicMask enters the methods that handle dynamic masking, i.e. ESMF_ArraySMM() and ESMF_FieldRegrid(). I would have naively thought that the ESMF_PredefinedDynamicMask class is used to generate an ESMF_DynamicMask object, which then can be passed into the unmodified existing methods mentioned above as usual. Maybe I am missing something about the actual use case, and why it necessitates the proposed, more integrated changes. I am sorry if this design aspect has already been discussed and I missed it.

The idea was that this would be something usable from python, via the C wrappers eventually. Since there is no dynamic masking option for the C interface to regrid that was one issue. The other issue is that right now to create dynamic mask you have to pass a Fortran procedure reference which would not be possible from python.

If we were just sticking to the Fortran API only then I agree with your analysis that what I've done here is overkill and what you propose would make more sense.

That said, I'm no expert in C so maybe I've overlooked something.

@theurich
Copy link
Member

The way I am thinking about this wrt C and Python interfaces is that we will expose the ESMF_DynamicMask class. It would be done similar to any other shallow ESMF class. We will NOT expose the current ESMF_DynamicMaskSet*() methods, which all take the dynamicMaskRoutine as required argument, and as you say, those don't make much sense on the C or Python level. Instead predefined dynamic masks are implemented on the Fortran side via a set of additional ESMF_DynamicMaskSet*() methods with required argument of type(ESMF_PredefinedDynamicMask), and those routines, as well as shallow class ESMF_PredefinedDynamicMask, are exposed to the C and Python API.

Does that make sense?

Thinking about it some more, we might not even really need the extra ESMF_PredefinedDynamicMask class. It woudl seem that additional ESMF_DynamicMaskSet*() methods I mentioned above could just directly take in the ESMF_PredefinedDynamicMask_Flag you have defined, and any additional info, to set up a predefined dyn mask. Seems pretty straight forward and clean.

@bena-nasa
Copy link
Author

bena-nasa commented Feb 18, 2025

@theurich I think I understand what you are saying and seems cleaner. I just have to wrap my head around the nuts and bolts. I'll look through the code try to understand how you expose some of these Fortran classes to C, but what would be perhaps the best example of another shallow ESMF class that you expose to the C (and therefore the python) interface that I could look at as an example? Maybe something like the time class?

@theurich
Copy link
Member

@bena-nasa e.g. look at ESMF_ArraySpec, and how it is routed through to the C API as ESMC_ArraySpec. You will see that it is first routed through to the C++ layer as ESMCI::ArraySpec, and then the C API is based on top of that.

@bena-nasa
Copy link
Author

bena-nasa commented Feb 18, 2025

@bena-nasa e.g. look at ESMF_ArraySpec, and how it is routed through to the C API as ESMC_ArraySpec. You will see that it is first routed through to the C++ layer as ESMCI::ArraySpec, and then the C API is based on top of that.

So the C API, routes through a C++ layer which is denoted by all the files that are named ESMCI, them from the C++ layer you are calling into the Fortran layer?

Is that general pattern? And can you say something about this FTN_X function?

@theurich
Copy link
Member

Yes, that is the general pattern. Files that start with ESMC_ should use strict C99 syntax, no C++. The C++ layer is implemented in files with ESMCI_ prefix (the I stand for "internal" - but that is a bit historical, it's the C++ internal layer).

The FTN_X() is a CPP (preprocessor) macro function that ensures the correct name mangling when calling from C++ into Fortran functions.

@bena-nasa
Copy link
Author

Where is that FTN_X defined out of curiosity? I can't seem to find it. Also I was looking through the developers guide and saw this remark
"ESMF is written in a combination of C/C++ and Fortran. Techniques used in ESMF for interfacing C/C++ and Fortran codes are described in the ESMF Implementation Report[#!bib:ESMFimplrep!#], which is available via the Users tab on the ESMF website. These techniques, which address issues of memory allocation, passing objects across language boundaries, handling optional arguments, and so on, are general and have been applied to multiple projects."

Does this report still exist?

@oehmke
Copy link
Contributor

oehmke commented Feb 18, 2025 via email

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

Successfully merging this pull request may close these issues.

3 participants