Fix ThermoPhase.atomic_weight typing mistake#2024
Conversation
interfaces/cython/cantera/thermo.pyi
Outdated
| @property | ||
| def element_names(self) -> list[str]: ... | ||
| def atomic_weight(self, m: int) -> float: ... | ||
| def atomic_weight(self, m: str | bytes | float) -> float: ... |
There was a problem hiding this comment.
The float really should be an int, as it refers to an index in the storage order. The float should be deprecated sooner rather than later, as it's extremely misleading.
Similar goes for bytes, which is just something where a pystr conversion was missed.
PS: It's actually interesting to see that type hinting exposes some of the more questionable argument types that we're still supporting .
There was a problem hiding this comment.
Indeed, I would have typed it as an int if it wasn't explicitly checking both int and float internally. I agree that it would be better to remove float as a valid option.
Regarding bytes, that's probably only "correct" because bytestrings are a thing, but I don't recall seeing anything which still returns bytes. It should be safe to drop them from all of the type annotations right now, and shouldn't be difficult to eliminate their remaining usage within the source code.
There was a problem hiding this comment.
Hi guys, I haven't had time to look at the implementation here, but I do want to comment so please don't merge. In general I would prefer, and I think it makes sense from a user perspective, to have the types be stricter than what the interface actually accepts. I suppose it's not strictly correct, but this isn't C or even Go where the code won't even compile with incorrect types, so user convenience matters more to me.
With respect to the float conversion, I'm fairly sure I did that to support a specific use case that escapes me at the moment, but for example might be pulling values out of a string which are often '10.0', so int() would fail. Aside from that, changing the function's types it accepts should go through a deprecation cycle that doesn't seem worth the churn.
There was a problem hiding this comment.
@bryanwweber I do agree that it would make sense to generally prefer type hints which match the intended usage, even if that is stricter than what the implementation will actually accept. We've made such changes in a few spots, such as using tuples for property setters which would technically accept any Sequence type.
I'd be fine with either rescoping this pull request to include removing most/all bytes throughout the stubs, or making a subsequent pull request to do so. This was just intended to be a super quick change when the inconsistency popped up in one of my codebases.
There was a problem hiding this comment.
@bryanwweber ... I see the convenience, but allowing a float for an index makes me shudder. I strongly recommend excluding this from being suggested by a type hint.
Similar goes for bytes ... we 'tolerate' it, but I absolutely don't want users to get ideas. The Python API uses str, full stop?
@TimothyEDawson ... either is fine by me, and I overall agree that type hints can be narrower than what is actually processed correctly. I only commented here as I realized that a float type hint was used for an index.
There was a problem hiding this comment.
I strongly recommend excluding this from being suggested by a type hint.
This is exactly what I'm suggesting. Even if the underlying implementation permits a float, we don't need to add that to the type signature. And we should not change the implementation to match the type signature, i.e., we should leave the implementation as-is.
There was a problem hiding this comment.
I’m a strong 👎 on float indices, but oh well. It’s a pre-existing condition, so as long as it doesn’t make it into type hints, I can live with it.
|
I went ahead and dropped |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2024 +/- ##
==========================================
- Coverage 76.47% 76.47% -0.01%
==========================================
Files 464 464
Lines 54938 54936 -2
Branches 9305 9305
==========================================
- Hits 42014 42012 -2
Misses 9798 9798
Partials 3126 3126 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ischoegl
left a comment
ischoegl
left a comment
There was a problem hiding this comment.
Thank you, @TimothyEDawson
I will still make a comment about the general utility of int for element_index et al.
>>> gas.species_index("CH4")
13
>>> gas.species_index(13)
13
>>> gas.species_index(12.99999999999999) # this is why allowing float is terrible
12
As I recently looked at this code, I know why the int is allowed here (it's there to check whether the index is within bounds), but it is not something I am overall endorsing. From that perspective, it may make sense to restrict the type hint to str alone.
3 participants
Changes proposed in this pull request
Missed that
ThermoPhase.atomic_weightcallsThermoPhase.element_indexbehind the scenes, so here's a quick fix.Checklist
scons build&scons test) and unit tests address code coverage