feat: IMPLEMENTS CHECKING FOR VARIABLE MATES #237
Conversation
On a latlon grid some variables have mates. This commit will produce a meaningful error if attempting to use `get_dataset` or `faces_dataset_to_latlon` to load a variable without its mate. See Github issue #234
fraserwg
changed the title
There was a problem hiding this comment.
This looks great! Thank you so much.
We will just need a test for this scenario. It would go in here:
xmitgcm/xmitgcm/test/test_llcreader.py
Line 76 in 7c5305c
Something like
def test_vector_mate_error(local_llc90_store):
store = local_llc90_store
model = llcreader.LLC90Model(store)
with pyest.raises(ValueError, match=r".* must also be .*"):
ds_latlon = model.get_dataset(type='latlon', varnames=['U'], iter_start=0, iter_stop=9, iter_step=8)| vnames.remove(mate) | ||
| except ValueError: | ||
| msg = 'If {} in varnames, {} must also be in varnames'.format(vname, mate) | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
This PR makes me realize we have some duplicative code paths in here. I'm trying to understand why we need to implement this in both _all_facets_to_latlon and faces_dataset_to_latlon. I think that faces_dataset_to_latlon should probably be refactored to call _all_facets_to_latlon. However, that is not your problem.
|
I've added the test function but am now getting test fails on the Xarray-master build, details below: =========================== short test summary info ============================
257
FAILED xmitgcm/test/test_llcreader.py::test_ecco_portal_load[2160] - ValueErr...
258
FAILED xmitgcm/test/test_llcreader.py::test_ecco_portal_load[4320] - ValueErr...
259
====== 2 failed, 276 passed, 3 skipped, 124 warnings in 106.82s (0:01:46) ======The tests weren't failing for the previous commit and I haven't changed the tests that are now failing. Also the other builds are passing (as are the tests on my local machine) -- could this be a problem with a new commit on the Xarray-master branch? Any ideas for where to start with debugging would be appreciated, I'm relatively new to the world of CI and pytest! |
removed test for vector mate error to determine whether CI problems are coming from the test or an xarray-master commit
|
Have just added a new commit with the test removed again. Looks like everything's passed suggesting the problem is indeed with my test function. |
|
Ok, all the problems have been sorted, it was just a missing pair of brackets :) |
|
Fantastic! Thanks so much. I appreciate your patience to sort out the CI failures. |
* feat: IMPLEMENTS CHAECKING FOR VARIABLE MATES On a latlon grid some variables have mates. This commit will produce a meaningful error if attempting to use `get_dataset` or `faces_dataset_to_latlon` to load a variable without its mate. See Github issue MITgcm#234 * test: ADDED TEST FOR THE MATE ERROR * test: REMOVED TEST FOR VECTOR MATE ERROR removed test for vector mate error to determine whether CI problems are coming from the test or an xarray-master commit * fix: ADDED TEST BACK AND CHANGED ARGUMENTS
On a latlon LLC grid some variables have mates. This commit will produce a meaningful error if attempting to use
get_datasetorfaces_dataset_to_latlonto load a variable without its mate.See Github issue #234