Added fix_inconsistent_variables

[braaannigan]

I added the function fix_inconsistent_variables to add the extra row onto drC. This function is called after the data is read from the mds files but before the data is read into the xarray dataset. Trying to change the drC array once it is in the xarray dataset causes problems, because xarray generates an error because the shape is different to what it expects.

I also changed read_grid=False to True on Line 165, as this seems to override the choice made when the user calls the function and so the grid wasn't being read. I'm not sure what this should be in general.

[braaannigan]

The indent on the new fix_inconsistent_variables was wrong in my first commit, I've modified this now

[codecov-io]

Current coverage is 91.44% (diff: 100%)

Merging #23 into master will increase coverage by 0.14%

@@             master        #23   diff @@
==========================================
  Files             4          4          
  Lines           529        538     +9   
  Methods           0          0          
  Messages          0          0          
  Branches        118        120     +2   
==========================================
+ Hits            483        492     +9   
  Misses           31         31          
  Partials         15         15          

Powered by Codecov. Last update 21be463...2739dfe

[braaannigan]

It's working now for both the test case and my own experiment now. It's great!

[rabernat]

@braaannigan this is fantastic! Thanks so much for getting this started.

I have a few minor suggestions for how I would prefer this work.

Also, we need a test of this new feature. You will have to add a new function to test/test_mds_store.py which simulates the situation you have encountered with your old version of mitgcm. (The easiest way to do this would probably be just to copy DRF.meta|data to DRC.meta|data in order to mock a data file of the correct length).

[rabernat]

Could you change this back? If you look more carefully at the code path, you will see that this is just part of the logic for how the dataset is put together. If read_grid is passed as True to open_mdsdataset, the function calls itself recursively. There are extensive tests (example to ensure that the options work properly. I am surprised this change didn't cause an error somewhere.

[rabernat]

Do you think you could make this function have the signature?

vname, dims, data, attrs = self._fix_inconsistent_variables(vname, dims, data, attrs)

Then you could do the check if vname=='drC' inside that function. (If false, you just return vname, dims, data, attrs with no modifications.) I would prefer this because it will keep the code path within __init__ much cleaner, and it will also generalize in case we have to do more such hacking on other variables.

[rabernat]

It should also be moved outside of __init__ and become its own method of the _MDSDataStore class.

[braaannigan]

Made those changes, it's still working for me

[rabernat]

@braaannigan, just curious, do you know how to run the test suite yourself?

py.test -v xmitgcm

Can be useful for development.

[rabernat]

This all looks good, but it still needs a test.

[braaannigan]

Hi @rabernat, this is all new to me!

In test/test_mds_store.py, what exactly needs to be added? Is it adding a new experiment to the dictionary on L60 with similar properties to global_oce_latlon?

Also, I'm going hiking for a few days, so I'll probably be offline from about 8am tomorrow until Tuesday. Hopefully we can get this sorted before then

[rabernat]

Hi @rabernat, this is all new to me!

Yes, it was new to me a few years ago too. Once you start testing, you will never go back. 😉

Add a function to test_mds_store.py that goes something like this

def test_drc_length(all_mds_datadirs):
    dirname, _ = all_mds_datadirs
    copyfile(os.path.join(dirname, 'DRF.data'), os.path.join(dirname, 'DRC.data'))
    copyfile(os.path.join(dirname, 'DRF.meta'), os.path.join(dirname, 'DRC.meta'))
    ds = xmitgcm.open_mdsdataset(
                dirname, iters=None, read_grid=True,
                geometry=expected['geometry'])
    assert len(ds.drC)==(len(ds.drF)+1)

You will also have to add from shutil import copyfile at the top of test_mds_store.py.

[braaannigan]

Great, that worked once I added 'expected' to the first line of the function. All tests passed. 2016-12-15 20:59 GMT+01:00 Ryan Abernathey <notifications@github.com>:

…

Hi @rabernat <https://github.com/rabernat>, this is all new to me! Yes, it was new to me a few years ago too. Once you start testing, you will never go back. 😉 Add a function to test_mds_store.py that goes something like this def test_drc_length(all_mds_datadirs): dirname, _ = all_mds_datadirs copyfile(os.path.join(dirname, 'DRF.data'), os.path.join(dirname, 'DRC.data')) copyfile(os.path.join(dirname, 'DRF.meta'), os.path.join(dirname, 'DRC.meta')) ds = xmitgcm.open_mdsdataset( dirname, iters=None, read_grid=True, geometry=expected['geometry']) assert len(ds.drC)==(len(ds.drF)+1) You will also have to add from shutil import copyfile at the top of test_mds_store.py. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKBpmdvdaNsm6Vn3dJxhrLR967su8GYSks5rIZwzgaJpZM4LN0Um> .

[rabernat]

Awesome! Your test (along with all the others) was run by travis CI on this pull request, and it passed. It will continue to be run every time someone makes a new pull request. This is how we ensure we don't break things as the package evolves.

You can also see that the code coverage checks also passed. This means that the new code you added also came with sufficient tests.

[braaannigan]

Cheers Ryan, thanks for your help!