-
Notifications
You must be signed in to change notification settings - Fork 21
Orb grid comp mod refactor rc #4332
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
base: release/MAPL-v3
Are you sure you want to change the base?
Conversation
|
Note that I played around a bit with refactoring the develop branch of this component over the weekend. In the end, the main thing that I wanted to accomplish appears to be too risky. Namely, I wanted to make the logic "grid agnostic" rather than special cases for latlon and CS. (And then I was going to fix the haloing issue.) |
tclune
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made several specific suggestions, but did not flag all of the commented lines that should be deleted.
…sed string vector for deferred length string vectores for Instrument and Satellite derived type members
|
@tclune All the changes have been made. |
|
|
||
| subroutine grid_get(grid, unusable, name, dimCount, coordDimCount, im, jm, rc) | ||
| subroutine grid_get(grid, unusable, name, dimCount, coordDimCount, & | ||
| im, jm, globalCellCountPerDim, rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to look into this. Our thinking is that we don't want user code to be getting global cell counts. Too much danger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Near as I can tell this is only being used to determine whether this is latlon vs cubed sphere. We want a better way to do that.
However, that fix should not be part of this PR. So I may end up approving and require you to go back and clean all this up when the other feature is available.
Am thinking something like
call mapl_GridGet(grid, class=class, _RC)
Where class is either a string {"latlon", "cubedsphere", ...) or possibly a simple derived type for type safety. (akin to ESMF_TypeKind_Flag).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imsize depends on IM_World, so in addition to knowing what type of grid this is, it looks like we actually need to know the global cell counts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tclune We keep bumping into this. There are several places where IM_WORLD is queried for. In FV3, we get around that by reading it from the config file.
tclune
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a few things to fix (for now). See inline comments for details.
- A few commented out lines to remove.
- Check whether comp_name is still needed - I think it was just fore use with "Iam"
- We need to redo some of this once GridGet can return the grid class.
- please open a new issue for that. And make fixing here a sub-issue.
|
Issue 4360 has been created to note the need to implement the GRID class feature. A sub issue to the issue has Las been created to note to use this feature in MAPL_OrbGridCompMod.F90 once it is available.
From: Tom Clune ***@***.***>
Date: Monday, February 2, 2026 at 8:01 AM
To: GEOS-ESM/MAPL ***@***.***>
Cc: Oloso, Amidu O. (GSFC-610.1)[SCIENCE SYSTEMS AND APPLICATIONS INC] ***@***.***>, Author ***@***.***>
Subject: [EXTERNAL] [BULK] Re: [GEOS-ESM/MAPL] Orb grid comp mod refactor rc (PR #4332)
CAUTION: This email originated from outside of NASA. Please take care when clicking links or opening attachments. Use the "Report Message" button to report suspicious messages to the NASA SOC.
@tclune requested changes on this pull request.
Only a few things to fix (for now). See inline comments for details.
1. A few commented out lines to remove.
2. Check whether comp_name is still needed - I think it was just fore use with "Iam"
3. We need to redo some of this once GridGet can return the grid class.
* please open a new issue for that. And make fixing here a sub-issue.
—
Reply to this email directly, view it on GitHub<#4332 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AB6NU7T2OY4ARH5ESO2766T4J5DB7AVCNFSM6AAAAACSXHU2NCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTOMZZGI2TIOBUG4>.
You are receiving this because you authored the thread.
|
| Use MAPL_BaseMod | ||
| Use MAPL_GenericMod | ||
| Use MAPL_Constants | ||
| Use MAPL_BaseMod, only: MAPL_PI,MAPL_DEGREES_TO_RADIANS_R8, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the constants, try
use MAPL_Constants, only: MAPL_PI, MAPL_DEGREES_TO_RADIANS_R8 etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - responded to this in main conversation. We certainly don't want to reference Base at all. As much as possible I think we just want use MAPL3 [, only: ...] Constants might be an important exception, but then it should be use MAPL_Constants [, only: ...]
| Use MAPL_Constants | ||
| Use MAPL_BaseMod, only: MAPL_PI,MAPL_DEGREES_TO_RADIANS_R8, & | ||
| MAPL_RADIANS_TO_DEGREES, MAPL_UNDEF, MAPL_DimsHorzOnly, & | ||
| MAPL_R4, MAPL_VLocationCenter, MAPL_FieldCreateEmpty, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAPL_VLocationCenter can be replaced with VERTICAL_STAGGER_CENTER from mapl3g_VerticalStaggerLoc`
| Use MAPL_BaseMod, only: MAPL_PI,MAPL_DEGREES_TO_RADIANS_R8, & | ||
| MAPL_RADIANS_TO_DEGREES, MAPL_UNDEF, MAPL_DimsHorzOnly, & | ||
| MAPL_R4, MAPL_VLocationCenter, MAPL_FieldCreateEmpty, & | ||
| MAPL_GridGetCorners, MAPL_FieldAllocCommit, MAPL_FieldBundleAdd, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a new MAPL_FieldBundleAdd in module mapl3g_FieldBundle_API
@tclune geom/VectorBasis.F90 has a GridGetCorners routine, but it is not included in geom/API.F90. Should we add it to API.F90?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grr. For now I suppose. I really want to consolidate things in GridGet where possible.
| MAPL_R4, MAPL_VLocationCenter, MAPL_FieldCreateEmpty, & | ||
| MAPL_GridGetCorners, MAPL_FieldAllocCommit, MAPL_FieldBundleAdd, & | ||
| MAPL_PackTime | ||
| Use MAPL_CommsMod, only: MAPL_AM_I_ROOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try MAPL_Am_I_Root from module mapl3g_Utilities_Comms_API
|
@pchakraborty Do we really want users to be so aware of internal module structure of MAPL? I think we want to aim for But there may be exceptions for specific cases such as constants. (Indeed, we may yet move constants into a separate repo.) |
We’ll have to do another pass closer to release where we replace the use statements with |
Not sure I agree. It ties our hands if we want to move something into a different module. Won't be much of that, but I'm not seeing much advantage to naming the specific module. |
Types of change(s)
Checklist
make tests)Description
Changed MAPL 2 to MAPL 3
Related Issue
4274