fix(statistics): allow at_port to be specified manually#1386
Conversation
|
The match statement is consistent with the minimal python version 3.10. https://peps.python.org/pep-0636/ |
|
yes, I like that feature very much! |
|
I added a Now, the complete documentation must be updated, too. |
|
@gianvicolux I think if you add a small release note this is ready for review. |
There was a problem hiding this comment.
Ok, with this it's good from my side.
There was a problem hiding this comment.
I didn't notices that at_port is just this:
if not at_port:
ports = [ports[0]]
which makes this PR a bug fix and not a feature.
Since we are touching it already, I'd make more changes and turn it into a bug fix and a new feature:
Which ports to consider:
- True: All ports of components
- False: Exclude first port ("bus"/"bus0")
- False: Consider only first port ("bus"/"bus0")
- str or list of str: Specific ports to include
Note: use port number only, without 'bus' prefix (e.g., '1' instead of 'bus1')
is mixing types and is not intuitive.
What about removing the boolean and just allowing a list of ports? The default could be set to None, which would map to c.ports. 'bus' and 'bus0' can be treated equally.
We are also now in 1.0 territory, which means:
- that we want to still allow
TrueandFalseand map them to all ports or just the first one, raising a deprecation warning. - don't wanna just raise a
ValueErrorforat_port="bus0"which very buggy worked before, but would fail now.
Also why kicking the 'bus' from 'bus1', but only allowing '1' but not 1 as int? Because that's what is returned from c.ports?
doc/references/release-notes.rst
Outdated
| Features | ||
| -------- | ||
|
|
||
| * New feature for retrieving statistics in respect to a specific port: ``at_port`` statistics argument is able to accept a single port or a list of ports (as the type arguments already said they would). Note: use port number only, without 'bus' prefix (e.g., '1' instead of 'bus1') (https://github.com/PyPSA/PyPSA/pull/1386). |
There was a problem hiding this comment.
Remove this file. This needs to go in docs/release-notes.md now and with markdown formatting so (- for bullet points, (<!-- md:pr 1386 -->) to link the pr and single '`' for code formatting.
when i touched it first, i considered it as a feature, i only noticed the docstrings late in the process. It's true that the docstring is not implemented.
I think, you need an option to specify all ports (across multiple component types) and this should not be the default, because it easily leads to double-counting and it was not the default up to now. What about as preferred type and thus deviating from
Sure.
This did not work before, it returned effectively the result of
Indeed, I like consistent naming and |
|
I am happy with:
And I agree, |
I think either numbers 0, 1, 2 would be good or actual attribute names: |
|
|
|
Hmm .. i thought some more. There will be recurringly the need to accept both types of inputs, type PortsLike = Literal["all"] | int | Sequence[int] | str | Sequence[str]
@dataclass
class Ports:
nums: list[int]
c: Component
@classmethod
def from_like(cls, ports: PortsLike, c: Component):
match ports:
...
return cls(nums, ct)
def __iter__(self):
return iter(self.nums)
def cols(self):
...and |
|
I think we should keep it simple. Returning a data class could cause confusion for a use case as simple as this. It also adds overhead. Accepting both types can be done everywhere with a simple |
|
Not sure what's the difference between internal, external. here. I'd expect the overhead to be negligible. You'll need at least two methods My thinking was that the dataclass would make that more discoverable, but the use is mainly internal anyway. The problem with ports also comes up frequently when formulating custom constraints, so making that easier would be a good thing. |
|
I still need something like this, is there a particular reason that you did close the PR? |
|
@lkstrp Happy if you can have a look. The diff was just against the wrong branch. |
|
Nice refactoring, there is one thing we have to be aware of though. If I see correctly, there is now an inconsistency how We could instead keep |
There was a problem hiding this comment.
The API get's also much better, thank you!
We could instead keep None as the
at_portdefault and centralize the implicit default in_aggregate_components.
I think this would be the cleanest
pypsa/statistics/abstract.py
Outdated
| ports = [ports[0]] | ||
| if at_port is False or at_port is None: | ||
| warnings.warn( | ||
| f"Passing `at_port={at_port}` is deprecated. Use `at_port=0` instead.", |
There was a problem hiding this comment.
Add version 1.2 and removed in 2.0, just check format of other deprecation warnings
I think, i only chose There i have and then a: if at_port is None:
at_port = "all" if bus_carrier else "bus0"Where do you think there is an inconsistency? |
|
Now I am confused. This PR is also on an old master where docstrings did not match the signature. You indeed don't introduce any inconsistencies, they have been in there before. In my understanding we should just default to if at_port is None:
at_port = "all" if bus_carrier else "bus0"
|
Is it? PyPSA/pypsa/statistics/expressions.py Lines 592 to 632 in e37b04f and on this PR: Where do you think is the inconsistency? If you mean the difference in treatment between capex, optimal_capacity and energy_balance in regards to Sounds like there might not be one. Maybe @FabianHofmann, you can clarify if there was intent. Or if you think your rule above:
should actually be general. |
|
@coroa sorry for being late here. the problem is that when specifying EDIT: Just wrote a test that checks the behavior, it confirms that this would be a breaking change. Fine if I push it? |
|
@FabianHofmann Feel free to push the test. |
Add name, ports, _as_port, _as_ports to return_from_first p CE38 roxy patterns so statistics work with NetworkCollection.
Set efficiency2 alongside efficiency to handle the case where a prior test adds bus2 to the Link component schema globally.
| Which ports to consider: | ||
| - None: Automatically set to "all" if bus_carrier is specified, otherwise "bus0" | ||
| - "all": All ports of components | ||
| - "bus0": Consider only first port |
There was a problem hiding this comment.
this line (coming from me) does not add a lot. happy to remove it again
Changes proposed in this Pull Request
For retrieving statistics in respect to a specific port, we'd want the
at_portstatistics argument be able to accept a single port or a list ofports (as the type arguments already said they would).
This means that for links with an efficiency of 0.9, they could be accounted for at the output capacity, for instance:
@FabianHofmann Does that make sense?
It's still marked as a draft, as we'll have to add documentation first.
Checklist
doc.doc/release_notes.rstof the upcoming release is included.