Minor improvement of snapshots handling#1091
Conversation
There was a problem hiding this comment.
This PR implements the first option. But I would rather do the second and get rid of snapshot as an index name altogether.
I agree with your decision to implement the first option for now (less intrusive). Perhaps with #1037 we can then rename the index level to timestep. The main reason why we used the term "snapshot" in the first place rather than "timestep" is because it could also be used for stochastic problem definitions. With #1037 the intended use is different.
In any case, I would remove the single index name for multi-indexed snapshots.
Yes!
Some tests fail because linopy can't handle a tuple of coordinates, both single indexed and multi indexed at the moment. This could be fixed here or in linopy. But let me know what you think first.
Sounds more like it should be addressed in linopy.
pypsa/utils.py
Outdated
| network_attribute: str, | ||
| index_name: str | None, | ||
| ) -> pd.Index: | ||
| def as_index(n: Network, values: Any, network_attribute: str) -> pd.Index: |
There was a problem hiding this comment.
index_name should be removed from docstring
pypsa/utils.py
Outdated
| """ | ||
| Decorator for deprecated function and method arguments. | ||
| Based on solution from [here](https://stackoverflow.com/questions/49802412). | ||
| Based on solution fr [here](https://stackoverflow.com/questions/49802412). |
@fneum, hmm, not sure I agree. If people are using index names in n.snapshots, this is a breaking change and can't be made backwards compatible. Probably not a problem for many users, but still. Merging this pr with option 1, and fixing it later with 1037, would just mean breaking things twice for those users. |
|
Ok, convinced. It's just an index name after all :) |
|
For the history books:
A follow up PR will clarify that. |
Closes #1082
Changes proposed in this Pull Request
Current state
n.snapshots.index.name-'snapshot'n.snapshots.index.names:['snapshot']n.snapshots.index.name-'snapshot'.n.snapshots.index.names-['period', 'timestep'].timestepandsnapshot. This can lead to all kinds of bugs, especially with the upcomingscenariodimension, and should be harmonised.Two ways to fix this
timesteptosnapshotn.snapshotsis bothsnapshotandperioddata, it is an arbitray access to the timestep data. But renamingn.snapshotsis very intrusive.snapshottotimestepn.snapshotswould be resolved.This PR implements the first option. But I would rather do the second and get rid of
snapshotas an index name altogether.Multiindex names
In any case, I would remove the single index name for multi-indexed snapshots. Since
idx.names==[idx.name]for a single dimension, we can drop it for multi-indexed snapshots. This would result in clear and unique dimension names for each dimension and fix the current behaviour.Some tests fail because
linopycan't handle a tuple of coordinates, both single indexed and multi indexed at the moment. This could be fixed here or inlinopy. But let me know what you think first.Checklist
doc/release_notes.rstof the upcoming release is included.