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

Feature/tclune/mapl3 latlongeomfactory #2309

Merged
merged 25 commits into from
Aug 22, 2023

Conversation

tclune
Copy link
Collaborator

@tclune tclune commented Aug 16, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

We could just wrap copy and modify LatLonGridFactor, but there are
several issues that should be altered in this pass.

First, the various bits of logic should be teased apart into separate
procedures and modules.  Also the logic can be cleaned in various
points.
@tclune tclune added 🎁 New Feature This is a new feature 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. Skip Changelog 📈 MAPL3 MAPL 3 Related labels Aug 16, 2023
Benjamin Michael Auer and others added 3 commits August 16, 2023 14:05
No clear benefit to compilation performance.
  - not much parallelism in this layer
  - gFTL containers are expensive to compile (esp) with NAG.
@tclune tclune marked this pull request as ready for review August 22, 2023 14:51
@tclune tclune requested review from a team as code owners August 22, 2023 14:51
tclune and others added 5 commits August 22, 2023 11:16
Just wouldn't stay away.
Corner case of non-periodic axis needs extra point for corner of last
process.  (A triple corner case {periodic, corner, last pe}.
…actory' into feature/tclune/mapl3-latlongeomfactory
@bena-nasa
Copy link
Collaborator

bena-nasa commented Aug 22, 2023

@tclune, I don't see a hook in the new GeomFactory for the equivalent of append_variable_metadata that was in the old factory

@tclune
Copy link
Collaborator Author

tclune commented Aug 22, 2023

@tclune, I don't see a hook in the new GeomFactory for the equivalent of append_variable_metadata that was in the old factory

I'm hoping that we do not need it in the new design. Basically Geom produces the baseline metadata. Other layers can append, but Geom is the initializer.

There are few other "missing" things that are in a similar state: I'm hoping I can avoid the need. If not ... we circle back and add them back.

Copy link
Collaborator

@bena-nasa bena-nasa left a comment

Choose a reason for hiding this comment

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

I see a lot of the helper procedures needed to specify the bounds for the output file are no longer here. Append_variable_metdata caught my eye first, that probably needs to be returned. The rest generate_file_bounds, generate_file_reference*, the question can the logic that those were implement be done in a way that it need not be attached to the specific grid for the IO layer.

@bena-nasa
Copy link
Collaborator

bena-nasa commented Aug 22, 2023

@tclune, I don't see a hook in the new GeomFactory for the equivalent of append_variable_metadata that was in the old factory

I'm hoping that we do not need it in the new design. Basically Geom produces the baseline metadata. Other layers can append, but Geom is the initializer.

There are few other "missing" things that are in a similar state: I'm hoping I can avoid the need. If not ... we circle back and add them back.

The one I would insist on being restored before I approve is append_variable_metadata.

The rest we can find other ways as you say.

The generator_file_reference and generate_file_bounds stuff, that could be put into a generic layer and queue off the fact that the cube grid is a multi-tile grid (making the assumption the tile index gets its own dimension). Some of the others such as get_grid_vars, get_file_format_vars could be derived from the metadata once that is made.

@bena-nasa bena-nasa self-requested a review August 22, 2023 20:03
Copy link
Collaborator

@bena-nasa bena-nasa left a comment

Choose a reason for hiding this comment

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

Seems zero diff to the old lat-lon grid factory.
I do think that the append_variable_metdata helper procedure needs restoration. That is the only helper procedure from the old factory I see that I could not work around or that could be derived from the metadata and keep our existing IO system working with this new layer.

@tclune
Copy link
Collaborator Author

tclune commented Aug 22, 2023

The one I would insist on being restored before I approve is append_variable_metadata.

Why does the metadata object need to be 1st created somewhere else? The create metadata routine is the former "append" plus the assumption that it will be first in the pipeline. I can't immediately recall how I convinced myself of this, but will certainly amend the interface as the use cases come in. Trying to keep it simple for the moment - just the stuff I can immediately see how it fits in the process.

@tclune tclune merged commit 650749b into release/MAPL-v3 Aug 22, 2023
8 checks passed
@tclune tclune deleted the feature/tclune/mapl3-latlongeomfactory branch August 22, 2023 21:04
@bena-nasa
Copy link
Collaborator

The one I would insist on being restored before I approve is append_variable_metadata.

Why does the metadata object need to be 1st created somewhere else? The create metadata routine is the former "append" plus the assumption that it will be first in the pipeline. I can't immediately recall how I convinced myself of this, but will certainly amend the interface as the use cases come in. Trying to keep it simple for the moment - just the stuff I can immediately see how it fits in the process.

Please go look at what the old append_variable_metdata is doing, I don't think you are understanding what it is for. This is NOT for appending the overall file metadata.
We add grid specific meta to the file variables, at least in the case of the cube, go look in the old factory. We add specific metadata to the variables so that panoply knows how to plot them. The append_variable_metadata was a way to know for a particular grid, if any extra metadata needs to be attached to the file variables we will be writing. Someone needs to know, if I'm a cube, every variable in the file needs these extra attributes.
In reality every factory that is not a basic lat-lon should be adding "coordinate" attribute to the variables according to CF. So there really does need to be a place in general if I'm grid foo, there may need to be some extra attributes that need to be added to the variables.

@tclune
Copy link
Collaborator Author

tclune commented Aug 22, 2023

OK - I understand. But still, it can wait for now. It will be straightforward enough to bring in when the time comes. Yes - we have to implement it for each factory subclass, but it's the same work now as it will be then, and my short term need is to use the factory to produce distinct grids so that generic can induce a regrid operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 📈 MAPL3 MAPL 3 Related 🎁 New Feature This is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants