Add capability to do unit conversations to capgen

dustinswales commented 9 months ago

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:

Addresses https://github.com/NCAR/ccpp-framework/issues/329 and https://github.com/NCAR/ccpp-framework/issues/403

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

gold2718 commented 9 months ago

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

dustinswales commented 9 months ago

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.

dustinswales commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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:

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)

climbfuji commented 8 months ago

@dustinswales Can you please merge in feature/capgen (need to resolve conflicts) before I review? Thanks!

climbfuji commented 8 months ago

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 commented 8 months ago

@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.

dustinswales commented 7 months ago

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

climbfuji commented 7 months ago

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

climbfuji commented 6 months ago

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

NCAR / ccpp-framework

Add capability to do unit conversations to capgen #504