-
Notifications
You must be signed in to change notification settings - Fork 6
add fallback data to caravan #492
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
base: main
Are you sure you want to change the base?
Conversation
… time, need to find another fix I think
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #492 +/- ##
==========================================
+ Coverage 79.15% 79.18% +0.02%
==========================================
Files 28 28
Lines 1871 1883 +12
Branches 165 168 +3
==========================================
+ Hits 1481 1491 +10
Misses 330 330
- Partials 60 62 +2
🚀 New features to boost your workflow:
|
|
before writing tests, I would like the feedback of Bart |
It might be simpler to define an environmental variable import os
if os.environ.get("CARAVAN_CACHE"):
# load from cache
else:
# load from 4TUThis would be a minimal change in the code, and would remove the need for users to pass things. Just make sure you download the caravan file from the HTTP link; https://opendap.4tu.nl/thredds/fileServer/data2/djht/ca13056c-c347-4a27-b320-930c2a4dd207/3/camels.nc so the file is identical to the OPeNDAP version. |
|
thanks for the insights! For now when I am working on Snellius I have caravan data mounted to something other then... Also, 1.3 GB that is not much :) |
|
I am also noticing how you use /3/ in your opendap url and not /1/ as it is in the code. |
|
@MarkMelotto the /3 indicates v3, we did some finetuning. Nice to see you're using the data! |
|
hey @Daafip ! I noticed that v3 only contains the camels.nc, not the other ones, is that correct? Please let me know! |
|
@BSchilperoort so now I would need to add: to a notebook in SRC to have this work if I understand correctly? |
| cache_dir = os.environ.get("CARAVAN_CACHE") | ||
| # Check if we want to load from 4TU or dCache | ||
| if cache_dir: | ||
| cache_dir = cache_dir.rstrip("/") # ensure no trailing slash issues |
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.
or do
xr.open_dataset(Path(cache_dir) / "{dataset}.nc")
or add that to the end of your |
|
@BSchilperoort alright thanks for the help. Then for new SRC machines the correct version will be installed already, but for the other we need to update them. |
I think you also need to make a new release, and update infra for it to be on new SRC machines. Note that for SRC you should be able to define environment vars for all users as well. For this you will have to modify the infra repo. See;
i.e. seems like modifying the Should be a good first issue to fix in the ansible setup so you can learn how that works 😉 You should follow Stefan's guide on how to test this locally; https://github.com/eWaterCycle/infra/blob/c02600be0b135577da9682d5b4986b81c8162ac5/VAGRANT.md |
|
phew thanks @BSchilperoort that is a lot of new stuff :D I think the idea was to have this as a fail safe and still use 4tu as default. So I guess that just leaves the release? |
OK then this is fine, yes. You can now override the 4TU default if required. Remember to also update the docstrings to reflect this new behavior. |
|
Hopefully getting the shapefiles will not cause issues... |
You could also use the cache for that. Should a straightforward addition |
v3 is updated 2025-09-16T06:19:23.356Z: I think I had a miscommunication with 4TU, the other files from v2 for the other netcdf files are the ones to use. |
Hmm that might be the issue we are facing with 4TU then? |
I don't thinks so, I cant quickly find the issue why we did the update but there was a reason for it.. |
BSchilperoort
left a comment
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.
some suggestions
thank you! |
Hi @BSchilperoort
Fixing issue #491 .
Since I am not that familiar with coding style of ewatercycle, I require your help.
The problem we are facing is that caravan on 4tu is not working reliably at the moment, so I made this fallback version myself.
The crop_ds change is now needed, because we now have date instead of time, need to find another fix I think, let me know what you think!
And please help me for the overall picture as to what i can approve.
Thanks!