-
Notifications
You must be signed in to change notification settings - Fork 24
Hadisd simplify 146 #147
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
Hadisd simplify 146 #147
Conversation
Pull Request Test Coverage Report for Build 17420997194Details
💛 - Coveralls |
|
I have done a lot of behind-the-scenes work on the static analysis on develop. Please rebase your branch, and ideally also make it compliant with 'ruff check' |
8e6b190 to
45e7093
Compare
…ownloads can be run as many times as we like
45e7093 to
d894f7f
Compare
|
I've made sure my changes are technically ruff compliant. Some notebooks fail ruff checks due to run magic & defining variable names in a separate notebook. I've told ruff to ignore this, specifically for the notebooks that cause the problem. Before hitting the merge button, I'll bring this up in our meeting tomorrow. |
|
I'm just working through this. The download notebook worked well, looks good to me. The conversion to Zarr issues a lot of warnings. It would be nice if it quieted the warnings after the first one. |
|
The tutorial depends on seaborn, which isn't currently part of the dependencies. I don't think PET wants to influence people's choice of plotting library, there are lots of sensible options and people should feel free to use whatever. I don't think it has many dependencies so I'm happy to add it, but you also might want to just consider if you want to redo the plotting with matplotlib or plotly. So no issue - but please confirm your choice, and add it to the dependencies if it's needed. I'll just manually install it for my testing purposes for now. |
|
I'm getting an error which seems to be related to the warning message I saw when converting to Zarr. The warning message is "ValueError: Invalid Zarr format 3 data_type: {'name': 'fixed_length_utf32', 'configuration': {'length_bytes': 48}}". Did you get that at any stage? |
|
I've made some changes to reflect your comments:
In the mean time I seem to have broken something whilst looking into why the Drop transform was getting rid of coordinates I didn't want it to. I've reverted back to the way I thought things were, but there is still a problem I need to look into. |
|
Thanks I'll take another look shortly. |
… numpy issues, also squeeze lat, lon and elev here instead of pipeline
… numpy issues, also squeeze lat, lon and elev here instead of pipeline
|
Right, should all be working again. Thanks again for your help! I've adjusted variables.Drop so it doesn't drop coordinates that I haven't asked it to. I did start thinking this might be intended behaviour, but I don't really think it's that useful. This meant that I did have to manually drop some coordinates (lat, lon, elev) as part of the pipeline, but I think it's better that I explicitly have to drop them. This was done so that .ToNumpy() would work correctly. The other numpy problem was that there was a station_id variable that only had a station dimension, no time. I have just dropped it during the preprocessing stage in notebook 2 since it's a bit redundant anyway. This leaves what I think is quite a sensible dataset. Perhaps give everything one more run to check it works and if it does then I'll merge it. I'll then have another PR soon that addresses the zarr chunking issues experience on HPCs. |
|
This is also working for me now. I'll merge this increment in, but I can see some possible improvements for another day. Thanks very much for the work! |
Put in some commentary about the dropping of coordinates with the Drop operator
Description:
Testing: