Fix capacity factor timeseries in convert.py #346
Conversation
The method convert and aggregate was adopted, so the capacity factor timeseries is returned and all functionality, such as return capacity and per_unit are kept.
|
thanks for the PR @TimFuermann, I will try to have a look at it soon |
|
Thanks again @TimFuermann, this makes really sense. The function itself is a mess (not your changes!), and it builds one critical core of all our analyses, which is why I postponed the review so long. But it looks good. I would like to have a second pair of eyes looking at it. @lkstrp could you also have a look if the main logics are preserved? It is mainly tracking the action steps within the function being subject to the set of arguments... there is no time pressure on this |
There was a problem hiding this comment.
One more sorry that this took so long @TimFuermann ! I full on lost that one.
But atlite will get more love from now on again.
I'll merge the fixed pre-commit and tests first, but this looks good.
Thank you!
atlite/convert.py
Outdated
| if capacity_factor or capacity_factor_timeseries: | ||
| if capacity_factor_timeseries: | ||
| res = da.rename("capacity factor") | ||
| else: | ||
| res = da.mean("time").rename("capacity factor") | ||
| res.attrs["units"] = "p.u." | ||
| return maybe_progressbar(res, show_progress, **dask_kwargs) | ||
|
|
||
| if capacity_factor or capacity_factor_timeseries: | ||
| if capacity_factor_timeseries: | ||
| results = da.rename("capacity factor") | ||
| else: | ||
| res = da.sum("time", keep_attrs=True) | ||
| return maybe_progressbar(res, show_progress, **dask_kwargs) | ||
| results = da.mean("time").rename("capacity factor") | ||
| results.attrs["units"] = "p.u." | ||
|
|
||
| else: | ||
| results = da.sum("time", keep_attrs=True) |
There was a problem hiding this comment.
Removing the return means that we no longer end the function here, which leads to undefined issues later. I know, this one is open for quite a while now, but could you have another look?
Otherwise this function is quite a mess and could get a general refactoring with your proposed changes included
There was a problem hiding this comment.
@lkstrp: I need to check it locally, but could you explain which undefined issues will occur later? I know the fix is not ideal, and was actually just done that kind of messy to not restructure the whole function.
There was a problem hiding this comment.
Further down, matrix is not defined. If you merge the current master, this will hopefully show up in the tests. They may not run through because of a missing cdsapi token, as this branch was derived earlier. But you can run them locally in any case using pytest ..
if index is None:
index = pd.RangeIndex(matrix.shape[0])But as you said, restructuring the whole function is better. If you have already looked at it, feel free. Otherwise I will deal with it at some point.
There was a problem hiding this comment.
@lkstrp I restructured the function and tested it. I commit it soon. Nevertheless, this would need to change some things in the origional test files, too and in the usage. I explain it with the commit, Best, Tim
There was a problem hiding this comment.
Sounds great! Thank you @TimFuermann
There was a problem hiding this comment.
@lkstrp Just uploaded a new version of how the convert and aggregate could be used more consistently. Nevertheless this includes heavy changes in the input flags, but allows for a more consistent way of using them. A default capacity layout is used if no capacities are provided. This is a constant 1MW per gridcell (which should be the same as looking for capacity factors). With this new implementation, capacity factors for regions, per gridcell, aswell as aggregated can be calculated.
I furthermore added minor changes to the pytest test_preparation_and_conversion, since the output format slightly changes now, as well as the way how the input flags need to be set.
- add per_gridcell aggregation - add default capacity layout - add consistent behaviour of capacity_factor or aggregation
The method convert and aggregate was adopted, so the capacity factor timeseries is returned and all functionality, such as return capacity and per_unit are kept.
Closes # (if applicable).
Change proposed in this Pull Request
Description
Motivation and Context
How Has This Been Tested?
Type of change
Checklist
pytestinside the repository and no unexpected problems came up.doc/.environment.yamlfile.doc/release_notes.rst.pre-commit run --allto lint/format/check my contribution