-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: read_arm_mmcr
fails when using a read-only input file
#888
base: main
Are you sure you want to change the base?
Conversation
act/io/arm.py
Outdated
if nc is not None: | ||
ds = xr.open_dataset(xr.backends.NetCDF4DataStore(nc)) | ||
# Change heights name to range to read appropriately to xarray | ||
if 'heights' in ds.dims: | ||
ds = ds.rename_dims({"heights": "range"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using heights later on for some calculations, this might break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zssherman Well, it doesn't, and I've tested it. Note that ds also has the 'heights' dim name changed prior to this PR.
As it stands, ARM currently holds only the b1 MMCR files, which have that dimension. The issue was with read-only files. Other than that, everything remained the same.
You can try the few lines of code in the issue I opened and see that it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i was curious on why we are checking for nans later on etc for height and if that would break if we had range, but I see we check for range as well. Thanks for the clarification.
Tests failing is unrelated to this PR |
…did not work on my mac after downloading data.
@isilber I was running into errors when using this change on my local machine but I've tested this out on read only files and seems to work. Would you be able to test this out first before we merge? |
read_arm_mmcr
when using read-only files as input #887