Conversation
|
Hey there, I'm still new to the github workflow, so please let me know how I can improve :) |
StuberSimon
force-pushed
the
glofas-support
branch
from
79878e8 to
33ef0bf
Compare
ekatef
left a comment
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.
| # Model and CRS Settings | ||
| crs = 4326 |
There was a problem hiding this comment.
I understand that this part is inherited from atlite itself but it would be great to cross-check it's applicable also for GloFAS
| "dis24": "discharge", | ||
| } | ||
| ) | ||
| # round coords since cds coords are float32 which would lead to mismatches |
There was a problem hiding this comment.
Is it the case also for GloFAS data?
| coords_as_attributes=[ | ||
| "surface", | ||
| "depthBelowLandLayer", | ||
| "entireAtmosphere", | ||
| "heightAboveGround", | ||
| "meanSea", |
There was a problem hiding this comment.
It looks the names of the variables must be updated
| rename_vars = { | ||
| "time": "forecast_reference_time", | ||
| "step": "forecast_period", | ||
| "isobaricInhPa": "pressure_level", | ||
| "hybrid": "model_level", | ||
| } |
There was a problem hiding this comment.
A revision may be needed also here
| Download data like Glofas from the Climate Data Store (CDS). | ||
|
|
||
| If you want to track the state of your request go to | ||
| https://cds-beta.climate.copernicus.eu/requests?tab=all |
There was a problem hiding this comment.
I guess the link needs update to match EWDS server (btw, it looks the doc string may need in era5.py may require an update as well):
| https://cds-beta.climate.copernicus.eu/requests?tab=all | |
| https://ewds.climate.copernicus.eu/requests?tab=all |
| If static is False, this function creates a query for each month and year | ||
| in the time axis in coords. This ensures not running into size query limits | ||
| of the cdsapi even with very (spatially) large cutouts. | ||
| If static is True, the function return only one set of parameters | ||
| for the very first time point. |
There was a problem hiding this comment.
I have noticed that EWDS server doesn't favour download a dataset for periods more than one year even for really small spatial areas (like 2x2 degrees). So, it looks even more crucial to have temporal disaggregation for GloFAS retrieval as compared with ERA5
| 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
| 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? |
3 participants
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.