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 request: define "positive" attribute for imports and exports #284

Closed
sdeastham opened this issue Apr 6, 2020 · 19 comments
Closed
Assignees
Labels
🎁 New Feature This is a new feature

Comments

@sdeastham
Copy link
Contributor

Differing conventions for the vertical attribute of fields and data can easily cause problems. This has been discussed for read-in of file data (see e.g. #66, #86) but not so much for internal handling and output. Some components - notably GEOS-Chem - need the atmosphere to be indexed "up" (i.e. level 1 is the surface air layer), but when requesting data from an import it is impossible to know whether the data is orientied "up" or "down". This results in a lot of data flipping as well as a lot of confusion. The same applies for exports; GEOS-Chem will naturally return data "up", but the HISTORY component will not understand this.

One possible solution would be to add a "positive" attribute to imports and exports. If exports are given a "positive" attribute (optional) on write, then when an import is requested, the field can either be left the same (if the attributes match - and I'd suggest having "down" as the default, for backwards compatibility) or flipped (if the attributes do not match). Similarly, when HISTORY requests data, it could either just write everything "down" (flipping fields defined as "up") or it could check for a "positive" attribute in the definition of the collection and act accordingly.

@sdeastham sdeastham added the 🎁 New Feature This is a new feature label Apr 6, 2020
@tclune
Copy link
Collaborator

tclune commented Apr 6, 2020

I agree with the need. Just one more wedge issue on top of several that have arisen recently.

Getting close to the point of a major rewrite of History, but we need to get these features working on a much shorter time table.

@sdeastham
Copy link
Contributor Author

After further discussion, it seems that the best option may simply be to always assume that any import or export is oriented as "positive=down". However, it would then be extremely useful to:

  1. Have ExtData reject any file which does not have a defined "positive" attribute, so that we can be confident that all data is oriented "positive=down" after ExtData is done working with it; and
  2. Add a feature to HISTORY which allows a collection to be specified with "positive=up", in which case the data would be flipped when written only and the "positive" attribute set to "up" in the definition of the file's "lev" dimension

@lizziel
Copy link
Contributor

lizziel commented Apr 7, 2020

Seb, to clarify, do you mean always assume that any import or export is oriented as "positive=down" in the absence of the positive attribute in the input file for imports and HISTORY.rc for exports?

@sdeastham
Copy link
Contributor Author

sdeastham commented Apr 7, 2020 via email

@lizziel
Copy link
Contributor

lizziel commented Apr 7, 2020

This sounds good to me.

@mathomp4
Copy link
Member

@bena-nasa @atrayano Was this fixed/completed?

Also, adding @WilliamJamieson as he is now looking at HistoryNG

@lizziel
Copy link
Contributor

lizziel commented Feb 23, 2021

I think @LiamBindle was going to look into this too. However, I never got back to him with any guidance before my leave (sorry!).

@LiamBindle
Copy link
Contributor

@lizziel No worries! I was, but I ended up just doing the vertical flipping offline (easy).

@sdeastham
Copy link
Contributor Author

I wondered about that! In the long term I think we still need to solve this though - unless we want to continue with met pre-processing forever, the only alternative to having a fix such as above in place (that I can see) would be to rewrite the GCHP interface and modify all of our met inputs to be GMAO-standard.

@mathomp4
Copy link
Member

Reexamined on 2021-Apr-12 with @bena-nasa and @tclune

@lizziel
Copy link
Contributor

lizziel commented Aug 2, 2021

Revisiting this old feature request in light of recent similar GCHP-related feature requests:

Those address the "positive" attribute handling for checkpoints and exports, but not imports. This feature request addressed imports but it looks like we got stalled in part because of hesitation to force users to update their existing data to include the vertical direction metadata. To get things moving again, how about this handling for imports:

If file specifies positive:down: do nothing
If file specifies positive:up: flip data arrays vertically so that MAPL imports are still stored internally as positive:down
If file does not contain positive:
* If main RC file contains IMPORT_POSITIVE_FILL: up: flip data as if the file specified positive:up.
* Else assume positive:down

The config file entry name is just an example of course. Also, this only applies to data that have lev as index since direction can be inferred if lev is pressure.

The goal is GEOS config and input files remain the same, and GEOS-Chem users could keep their old files as is. Moving forward all new GEOS-Chem input files would specify positive.

@sdeastham, @LiamBindle, thoughts?

@LiamBindle
Copy link
Contributor

@lizziel I like that handling. It's backwards compatible but cleanly allows for the introduction of "positive" handling.

I would prefer IMPORT_POSITIVE_FILL be required though (with a default value of down in our config files). This is an aside, but in general I'd prefer to favor mandatory settings over optional settings because it's hard to keep track of what settings actually exist. The configuration files are already necessarily complicated, so I think settings should be forced to be explicit. Just my 2c though.

@LiamBindle
Copy link
Contributor

Hi all, I'm also revisiting this. I've been asked to get GCHP working with native metfields. I need to either implement a flipper, or as Seb suggested, rewrite the GCHP interface for top-to-bottom level indexing.

Also, as a side note, IIUC "positive" up/down refers to the direction of the coordinate's values which isn't necessarily the same as the direction of level indexing. For example, GEOS's output is positive=down with level coordinaes of 1..72; for GCHP we need to flip the vertical index but positive="down" is still the correct description.

Since there isn't an immediate solution, does anyone have ideas for a kludge that we could replace with the proper fix in the future? @lizziel @sdeastham @tclune @bena-nasa (and others)

@lizziel
Copy link
Contributor

lizziel commented Aug 30, 2021

I wonder if my suggestion above could be implemented quickly given the History update was very fast. @tclune, @bena-nasa, what do you think of my suggestion a few comments up? As Liam just pointed out we also need to take into account the level coordinates. It seems like the easiest solution is have MAPL figure it out upon read based on lev:positive and level coordinate values, and then always store the imports with the same convention (positive down, level coordinates 1..72).

If that will take a while or needs more discussion, perhaps a quick fix for native metfields is to take advantage of the fact that all of them have the same convention. A pretty simple kludge would be add CPP switch for using native met, and then apply the appropriate handling which we know will be the same for all of them. That would be easy to delete later on when we have an upstream feature to better handle this.

@bena-nasa
Copy link
Collaborator

bena-nasa commented Aug 31, 2021

@lizziel @sdeastham @tclune
Lizzie,
Is this the suggestion you are talking about:
"Those address the "positive" attribute handling for checkpoints and exports, but not imports. This feature request addressed imports but it looks like we got stalled in part because of hesitation to force users to update their existing data to include the vertical direction metadata. To get things moving again, how about this handling for imports:

If file specifies positive:down: do nothing
If file specifies positive:up: flip data arrays vertically so that MAPL imports are still stored internally as positive:down
If file does not contain positive:
If main RC file contains IMPORT_POSITIVE_FILL: up: flip data as if the file specified positive:up.
Else assume positive:down"

This should be possible to quickly implement when reading the import and internal restarts (the code where they are read makes no distinction). So the bottom line would be that MAPL still stores things internally in the GEOS convention, just allows for restarts to be stored in the opposite manner?

@lizziel
Copy link
Contributor

lizziel commented Aug 31, 2021

Yes, that is the suggested handling I am talking about.

So the bottom line would be that MAPL still stores things internally in the GEOS convention...

Correct.

...just allows for restarts to be stored in the opposite manner?

No, the internal, import, and export states would all be stored internally in the GEOS convention. When assigning GEOS-Chem arrays within the GEOS-Chem grid comp we'd then be able to apply consistent handling when extracting data from all three MAPL states.

For this update it is important that we have a config option for specifying a fill value for lev:positive. Many of our met files do not contain that attribute but store data as positive: 'up'. We need to make sure the data in those files is correctly transformed into the GEOS convection. Whether that config field would always be required to run, e.g. in GEOS too, would be up to you.

@bena-nasa
Copy link
Collaborator

The export states are never written and then read unless there is some use case I'm not aware of so there is no action that would need to be done to them. I'd probably have to as part of generic initialize chem the config for the attribute and set it in the internal/import states, as the place I have easy access to the file metadata, the RC file is not available.

@lizziel
Copy link
Contributor

lizziel commented Aug 31, 2021

Yes, sorry, my statement was too general. For the case of exports we fill them with GEOS-Chem array data so having them stored in the GEOS convention prior to write is what I meant. There was further work for exports to allow specifying positive in output file, but that's a separate github issue and I think you said it is already in a MAPL release now.

@bena-nasa
Copy link
Collaborator

I'm closing this issue as this has been addressed for History and checkpoint/restart as requested via other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎁 New Feature This is a new feature
Projects
None yet
Development

No branches or pull requests

8 participants