search

spencerahill / aospy

Python package for automated analysis and management of gridded climate data
Apache License 2.0
83 stars 12 forks source link

Fix failing tests due to auto_combine deprecation #324

Closed spencerahill closed 5 years ago

spencerahill commented 5 years ago

Closes #323

The failures related to xarray's deprecation of auto_combine, which has been superseded by the combine_by_coords and combine_nested top-level functions (and associated combine='by_coords' and combine='nested' options for open_mfdataset.

Took the liberty to clean-up a few style things unrelated to the actual meat of the issue while I was at it; apologies for murking up the diff in the process.

More substantively, I refactored _prep_time_data out of data_loader and into utils.times, since that seems more appropriate from an organizational standpoint. But its tests rely on some of the fixtures defined in test_data_loader, and so for now I just hard-copied those into test_utils_times. Should we move our various fixtures somewhere central, e.g. test/__init__.py. to avoid this problem in the future? (This could be a can of worms, so I'm also not opposed to walking back this for now, which wasn't in the scope of the original issue in the first place.)

Also, I removed the logic in the test of ensure_time_as_index that checks that the input dataset it unmodified. The only place we call it is to rewrite it anyways: ds = ensure_time_as_index(ds), so this seemed superfluous.

@spencerkclark (been a while!) would you mind sharing your $0.02 on this when you get a chance? TIA.

spencerahill commented 5 years ago

At some point, like xarray has done, we may consider moving our CI completely to Azure pipelines, so that we can have all platforms tested under one umbrella.

Fully agree. I'll open an issue to track it.

Regarding duplication of test fixtures, is there anything stopping you from importing them directly from test_data_loader.py within test_utils_times.py?

This led me down the right track. It turns out that importing fixtures from another module isn't ideal, because you have to also import every fixture that the given fixture calls, recursively. And the linter thinks that the imports aren't used. But some googling led me to pytest's docs, which recommend putting fixtures in a conftest.py file, which I have now created.

Moving forward I think it makes sense to put fixtures there as much as possible, and it might also be useful to do a refactor of our existing tests. (I suspect we're defining sample Datasets numerous places, when we could just have one fixture to do that.) But that's probably low priority...if it ain't broke, as they say...

Still haven't finished this PR, as there are other warning remaining to fix.

spencerahill commented 5 years ago

@spencerkclark this is ready for a final check over if you don't mind. Test suite now runs with zero warnings. TIA.

spencerahill commented 5 years ago

Thanks @spencerkclark

On the topic of formatting, it seems a number of your changes are taking things in the direction that black would go

I agree. I have played around with black not in aospy but applied to my own scientific code, and I've found in that case it's too restrictive: e.g. in expressions of physical equations, it obliterates line breaks that have physical meaning, etc.

But that's not an issue for aospy, and as usual if xarray does it we probably should to.