Implement GLOFAS dataset#498
Conversation
|
Hey there, I'm still new to the github workflow, so please let me know how I can improve :) |
79878e8 to
33ef0bf
Compare
ekatef
left a comment
There was a problem hiding this comment.
Great @StuberSimon! Looks a very good progress, and happy to see GloFAS is being implemented. Have taken a liberty to do a preliminary code review and added a few comments hoping to assist you in getting used to github. My general impression that you are getting things perfectly right and the implementation looks quite neat in general.
| monthly_requests : bool, optional | ||
| If True, the data is requested on a monthly basis. This is useful for | ||
| large cutouts, where the data is requested in smaller chunks. The | ||
| default is False |
There was a problem hiding this comment.
Given EWDS limitations, I'd expect monthly_requests=True could be a more reasonable default. Could be good to test if monthly_requests=False actually works for GloFAS retrieval
There was a problem hiding this comment.
As explained in the comment above, monthly_requests=False does work quite well for GloFAS retrieval.
monthly_requests=True makes the retrieval slower, as atlite has to wait for each api call seperately.
So I would suggest to not change the default to True.
But happy to discuss this further if you want :)
| Get inflow time-series for `plants` by extracting the discharge time series for | ||
| the nearest grid points. |
There was a problem hiding this comment.
Completely agree that it's worth to make sure the naming reflects different nature of variables in ERA5 and GloFAS datasets.
It could be a good idea also to mention both datasets in docstrings of the respective functions (that is mention here that _hydro_from_discharge is intended for usage on GloFAS data)
Thank you @StuberSimon, looks a great contribution! From my users' perspective, I strongly confirm that having GloFAS would be an amazing feature 🙂 Have added a few preliminary technical comments while leaving more in-depth analysis for @euronion who has much deeper understanding of For the next step, do you need any support? |
|
Hi @StuberSimon, @ekatef, and @euronion First off, great work on this PR. I completely agree with @ekatef that having GloFAS natively integrated is a fantastic and much-needed feature for the community. I'm jumping into this conversation since @Asdominet34 and I have worked on improving the hydromodeling in PyPSA-Eur, by firstly connecting it with GloFAS, "calibrating" it, and then validating it with the real hydro production in Europe. Before pushing the work, we would like to finalize the pumped-storage logic and the working paper we had in mind. I think that there is space for collaboration. What do you think about organising a call next week? |
|
Hi @ValeMTo that sounds great! |
Hi @simon, thanks for your message! Unfortunately next week is quite busy on our side as well, so we won’t be available then. We’ll get back to you shortly with our availability starting from the following week. Sorry again and looking forward to connecting! |
coroa
left a comment
There was a problem hiding this comment.
Sorry for dragging my feet on this PR. Actually, i retract my earlier statement that this has no space within atlite. I think you showed that this can be accommodated here in a way that is useful and backwards compatible.
A couple of changes would be good:
- abstract common code shared between era5.py and glofas.py in some cds_helper.py module
- clean up the coordinates and variable names in use (ie. comments by @ekatef )
- maybe an evaluation of potential heuristics for selecting the correct discharge grid cells
| for plant in plants.itertuples(): | ||
| # Extract the discharge time series for the nearest point | ||
| inflow.loc[dict(plant=plant.Index)] = discharge.sel( | ||
| x=plant.lon, y=plant.lat, method="nearest" | ||
| ) |
There was a problem hiding this comment.
My previous look into these datasets suggested that it is quite easy to miss the correct river cells due to small misalignments in the datasets, so i'd expect one would like to have some more sophisticated find closest river cells which are actually part of the river, rather than find closest cell heuristic. And that this would need testing.
There was a problem hiding this comment.
After some quick thinking probably the best thing would be a snapping to the largest value in the static uparea map (gridded data with the total catchment area of each cell).
That is available from here: https://confluence.ecmwf.int/display/CEMS/Auxiliary+Data
This could also be a pre-processing step on the powerplant dataset for which we provide a helper function that has to be called before.
The only problem is then that when your plant is right before a fork you would latch onto the larger river.
There was a problem hiding this comment.
Hey @coroa thanks for the tip!
I found solving this (especially the fork/larger river problem) quite difficult.
I think I found a working, though not perfect and quite complicated solution.
As the full solution requires some major code additions, I'm not sure if it fits in atlite or this pr.
You can find a first draft and results of these changes here: StuberSimon#1
Note that those changes will still need some preprocessing done in pypsa-earth:
- Calculating expected upstream area of plants (needs head from GHR)
- Getting river names for plants and mapping them (from GHR to OSM)
- Passing those to atlite snapping function, which will combine area matching and river name snapping to find the right glofas cell for each plant
-> This shouldn't be too hard, as GHR and OSM are already used in pypsa-earth. Only the name mapping needs to be done by the user.
I would suggest one of the following options:
- Keep snapping to nearest cell, leave it for future improvements to be solved
- Option 1, implement solution for this in pypsa-earth
- Add only largest upstream area snapping functionality, leave fork/larger river problem to future improvements
- Implement whole package (upstream area and river name matching) in this pr
- Option 1, but I'll implement the whole package in a follow up pr
I would personally prefer option 5. It would keep this pr small and provide a more accurate river cell snapping. I am planning substantial hydro functionality improvements in pypsa-earth after this, and fixing the snapping problem before would be important for those upgrades too.
I would love to hear what you and others (@ekatef) think about that!
|
Hello! @StuberSimon if you'd need any support with additional testing, happy to assist. We are currently working with GloFAS for one of the projects, and there is a bit of struggle exactly with finding the river route as @coroa has mentioned. |
# Conflicts: # .gitignore # atlite/convert.py # atlite/datasets/era5.py
| else: | ||
| ds = xr.open_dataset(target, chunks=sanitize_chunks(chunks)) | ||
| if tmpdir is None: | ||
| add_finalizer(target) |
There was a problem hiding this comment.
the signature of add_finalizer suggest that here is a bug, should be add_finalizer(ds, target). encountered while merge the latest master on this branch. will push the fix
FabianHofmann
left a comment
There was a problem hiding this comment.
to give this a little push, I am merging this latest master into here. I will follow up with more comments later
- fix add_finalizer(ds, target) call in glofas retrieve_data (was missing ds) - make zip extraction tmpdir-safe via Path(target).parent - align cds_helper helpers with master's typed, lint-clean versions - add type annotations, Returns/Raises docstrings, and logging/pathlib fixes
- restore hydrobasins as 3rd positional arg, make module/time keyword-only so existing positional callers (cutout.hydro(plants, basins)) keep working; collapse the duplicated auto/glofas/era5 dispatch into one path - point era5 request-tracking url back to the CDS server (was pointing at the EWDS/GloFAS server) - drop code-restating comments in glofas retrieve_data
Refs #450
Changes proposed in this Pull Request
Checklist
doc.environment.yaml,environment_docs.yamlandsetup.py(if applicable).doc/release_notes.rstof the upcoming release is included.