-
Notifications
You must be signed in to change notification settings - Fork 33
(Towards #868) Metadata support for fields with nlayers and ndata #3209
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
Draft
arporter
wants to merge
16
commits into
master
Choose a base branch
from
868_nlayers_ndata_mdata
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
234a26a
#868 add text describing nlevels and ndata
arporter 060a1ea
#868 update the rules for kernel arguments
arporter 716c908
#868 change nlevels to nlayers
arporter 5a04ba3
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter e878688
#868 extend docs to allow for naming of nlayers values
arporter 5cdb284
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter 3d4e9b9
#868 update nlayers docs
arporter a0f3f15
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter 5db865a
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter a0132df
Merge branch '868_nlayers_ndata_mdata' of github.com:stfc/PSyclone in…
arporter 1bfa1af
#868 update the nlayers text to make it clear that named values are j…
arporter a6833a1
#868 add initial code to get nlevels metadata for a field arg
arporter 0f54ac6
Merge branch '868_nlayers_ndata_mdata' of github.com:stfc/PSyclone in…
arporter b5751bb
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter 6e5b79a
Merge branch 'master' into 868_nlayers_ndata_mdata
arporter 892245f
#868 update nlayers documentation to remove special gh_runtime tag
arporter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 LFRic atmosphere, It would be useful to have the same flexibility for giving named values as is done for NLAYERS: in fact, it is more useful to have the flexibility here than for NLAYERS as there is less mixing and matching of layer numbers.
We have a lot of physics kernels with ~100 fields with a handful of multidata choices distinguished by ANY_DISCONTINUOUS_SPACE_1, ANY_DISCONTINUOUS_SPACE_2 etc. If they are converted to GH_RUNTIME we would need separate dofmaps etc. for each of the many fields rather than for each of the ANY_DISCONTINUOUS_SPACE choices. Example kernel here [MOSRS password protected]:
https://code.metoffice.gov.uk/trac/lfric_apps/browser/main/trunk/interfaces/physics_schemes_interface/source/kernel/bl_exp_kernel_mod.F90
Uh oh!
There was an error while loading. Please reload this page.
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.
Just commenting that I agree with this point! In fact I think the most common situation is that
NDATAwill be a runtime variable, but there might be many multidata fields being passed as arguments the same kernel, but all using the sameNDATAvalue.I'm not sure what the best solution to this is... because the
NDATAvalues are science-related, I don't think we want any specific values hard-coded inarguments_mod.Could we define a local variable for
NDATAto take? e.g.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.
This is the sort of thing I had envisaged (though originally I was thinking of a direct but more meaningful replacement for ANY_*SPACE entries).
One of the challenges of moving away from ANY_DISCONTINUOUS_SPACE settings was deciding how and where to name scientifically-meaningful versions given that
arguments_modis in core. Onecouldhave a physics module to store them which would enable a common set to be used by several kernels.I think they would need to be initialised so as not to cause any compiler warnings? If so, that's another reason for hiding them in a module.
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.
Great, that's the same as I was imagining for NLAYERS so it's straightforward to do from a PSyclone point of view. I can see your issue with names in core and physics but I'll let you wrangle that :-)
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.
Something else to consider that Tom M has just mentioned is that we have two types of multidata field. Those with
ndata_firstand those without. We may also need to capture that