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

Add capability to do unit conversations to capgen #504

Merged

Conversation

dustinswales
Copy link
Collaborator

@dustinswales dustinswales commented Oct 11, 2023

This PR adds automatic conversions for supported unit and type transforms.
Also, included is a new ccpp variable property to indicate a scheme variables vertical orientation vertical, top_at_one.
As default, top_at_one is set to .false.(GFS ordering convention).
Adding top_at_one = .true. to a variable in a schemes metadata file will trigger automatic array flipping:

The var_aciton_test was modified to test these new functionalities:
image

Addresses #329 and #403

Conversions supported:
https://github.com/NCAR/ccpp-framework/blob/main/scripts/conversion_tools/unit_conversion.py

@gold2718
Copy link
Collaborator

Last week: "meet with us and share how you would have approached it"
This week: PR
Did I miss something?

@dustinswales
Copy link
Collaborator Author

dustinswales commented Oct 11, 2023

Last week: "meet with us and share how you would have approached it" This week: PR Did I miss something?

Fair point.
This is a draft on how we approached it, which may or may not be inline with your thinking on how to use the VarAction class for unit conversions. Hopefully this makes our upcoming conversation more focused.

scripts/metavar.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
@dustinswales dustinswales changed the title Feature capgen unit conversions TESTING ONLY: Feature capgen unit conversions Oct 20, 2023
@dustinswales
Copy link
Collaborator Author

@DomHeinzeller @gold2718 @peverwhee
As we discussed, I jettisoned what I was doing w/ the VarAction class and added the unit-conversions to the compatibility object.
This works for unit conversions, but may need some changes to flip the vertical orientation.
A related question, how do we recognize when a variable needs to be flipped in the vertical? (New attributes in host and suite for convention?)

@gold2718
Copy link
Collaborator

@DomHeinzeller @gold2718 @peverwhee As we discussed, I jettisoned what I was doing w/ the VarAction class and added the unit-conversions to the compatibility object.

It looks like you added a VarCompatObject to what the unit conversion work. Is that what you meant?

This works for unit conversions, but may need some changes to flip the vertical orientation. A related question, how do we recognize when a variable needs to be flipped in the vertical? (New attributes in host and suite for convention?)

This has been recognized for quite a while. I know there was a discussion in a Google doc many years ago but I no longer have access to that (if I could even remember where it was).

However @grantfirl started this back up in August and I would like to see some progress on this so please refer to the Discussion related to vertical coordinate interoperabilty. I have added a bit of history to that.

@dustinswales dustinswales changed the title TESTING ONLY: Feature capgen unit conversions TESTING ONLY: Feature capgen transforms Oct 25, 2023
@dustinswales
Copy link
Collaborator Author

Hey @gold2718 @DomHeinzeller @peverwhee
I think this is at a point where I'd like to get some feedback from the team.

  • Unit, kind, and vertical loop ordering conversions are working using VarCompatObj.
  • There's a new variable attribute, top_at_one, which is used to trigger vertical loop flipping.
  • I modified the test/var_action_test to trigger these three types of conversions:
    Screenshot 2023-10-27 at 11 43 00 AM

Questions/Comments/ToDo:

  • I kinda obliterated the forward/reverse_transform functions in VarCompatObj to make this work. I don't the idea behind why they were designed the way they were(My guess is in the context of a vertical loop substitution?). Also, they weren't used anywhere other in doctests, so I felt that messing with them was okay.
  • Modify test to have vertically dependent fields (to test vertical flipping). This is trivial.
  • dimension reordering action (e.g. [i,j] -> [j,i])? This is built into varCompatObj, but so were the other transforms, which needed refactoring to work. Is this a priority?
  • This is not as clean as I'd like and I'm not sure how to best organize this? Specifically, the declarations and allocations of local variables occur at the Group level, whereas the actions are written at the Scheme level. I think it would be cleaner if all transform related business happened at the same place. Maybe this is not feasible? (I haven't looked hard into this)

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

See comments / questions below.
Also, note that the current var_action unit test suite does not all pass. This suite needs a careful look to make sure it is still correct and also should be expanded for more complex tests.

scripts/framework_env.py Outdated Show resolved Hide resolved
scripts/metavar.py Outdated Show resolved Hide resolved
scripts/suite_objects.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I realized that I forgot to hit the "submit review" button. Most of it is outdated by now, but for the records ...

scripts/suite_objects.py Outdated Show resolved Hide resolved
scripts/suite_objects.py Outdated Show resolved Hide resolved
@climbfuji
Copy link
Collaborator

In general, it would be nice to have more inline documentation in form of comments and/or docstrings, especially for newly added code. capgen is a pretty complicated piece of python code, any documentation will be greatly appreciated.

@dustinswales
Copy link
Collaborator Author

@DomHeinzeller I added some more inline documentation and addressed your comments. Since you reviewed, I refactored a bit to be more closely aligned with #512.
@gold2718 @peverwhee @mkavulich This is ready for review.

scripts/var_props.py Outdated Show resolved Hide resolved
dustinswales and others added 2 commits December 5, 2023 10:11
…ked_data_ci_tests

ccpp-prebuild: add blocked data tests and run prebuild tests in CI
Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

Thanks Dustin! A couple things.

Also: would it make sense to rename the var_action_test to something else? var_compatibility_test?

scripts/var_props.py Outdated Show resolved Hide resolved
scripts/var_props.py Outdated Show resolved Hide resolved
scripts/var_props.py Show resolved Hide resolved
scripts/suite_objects.py Show resolved Hide resolved
@dustinswales
Copy link
Collaborator Author

@peverwhee Comments addressed. Thanks!
I went ahead and renamed the test var_compatability.

Copy link
Collaborator

@peverwhee peverwhee left a comment

Choose a reason for hiding this comment

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

looks good!

@climbfuji
Copy link
Collaborator

@mwaxmonsky @gold2718 When you have time, can you please review this PR? Thanks very much in advance.

@climbfuji climbfuji linked an issue Dec 20, 2023 that may be closed by this pull request
@climbfuji climbfuji changed the title Feature capgen transforms Add capability to do unit conversations to capgen Dec 28, 2023
@climbfuji climbfuji linked an issue Dec 28, 2023 that may be closed by this pull request
@climbfuji
Copy link
Collaborator

Pinging all remaining reviewers. This PR has sufficient approvals and we are planning to merge this PR on Friday, January 12 2024.

@climbfuji climbfuji merged commit 800ea07 into NCAR:feature/capgen Jan 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants