Vertical transform bugfix for variables with only a vertical dimension #531
Conversation
peverwhee
requested review from
climbfuji,
gold2718,
dustinswales,
mwaxmonsky,
grantfirl and
mkavulich
as code owners
| if var1_dims or var2_dims: | ||
| _, vdim_ind = find_vertical_dimension(var1_dims) | ||
| if (var1_dims != var2_dims) or (vdim_ind >= 0): | ||
| if (var1_dims != var2_dims): |
There was a problem hiding this comment.
I guess we have not needed that or since var[12]_top was brought in. But that makes me wonder what is going on with line 938. That line is redundant, is it supposed to be something else (e.g., self.__equiv = False)?
There was a problem hiding this comment.
@dustinswales do you have insight on this. It looks like "self.__compat = True" is happening within the "if self.__compat" block.
There was a problem hiding this comment.
We can follow up on this after merging the PR, leaving the comment as unresolved.
scripts/var_props.py
Outdated
| @@ -808,6 +808,19 @@ class VarCompatObj: | |||
| _DOCTEST_RUNENV) #doctest: +ELLIPSIS | |||
| <var_props.VarCompatObj object at 0x...> | |||
|
|
|||
| # Test that a 2-D var with no vertical transform works | |||
There was a problem hiding this comment.
Why is a variable with only a vertical dimension a 2-D var?
There was a problem hiding this comment.
this is a constant point of confusion for me - I'm mirroring the tests above and below that say that a 3D variable has a horizontal and vertical dimension and a 2D variable has only a horizontal dimension.
There was a problem hiding this comment.
Well ... it's super non-intuitive I suppose, but: In most modeling systems, a single horizontal dimension still represents a 2-dim distribution of grid columns (just stored in one long vector). This makes a lot of sense for CCPP, which is supposed to work with models that have regular and irregular grids. For example, in MPAS there's only one horizontal dimension, because the data is completely irregular. The convention/requirement therefore is that any model collapses the horizontal dimension(s) into one horizontal dimension for CCPP.
Unfortunately, there are other models like single column models where one could have individual columns for whatever horizontal distribution (none at all, same column, or meaningless, or actual columns picked from a 2-d distribution from another model); for example to test perturbation of physics for the same column.
For the vertical dimension, there's of course only one. So fields that have no horizontal dimension are definitely 1-d and not 2-d.
There was a problem hiding this comment.
Thanks Dom! That makes (vague) sense. I've updated the comments.
update comments
climbfuji
changed the title
|
Going to merge this - it has three approvals from @dustinswales @gold2718 and @climbfuji! |
Enable 1D variables with only a vertical dimension by removing code that always adds a dimension transform.
var_props.py:
User interface changes?: No
Testing:
test removed:
unit tests: Add doctests for vertical-only coordinate (not flipped and flipped)
system tests:
manual testing: