Skip to content

fix: added branch for value_in to handle Celsius#108

Merged
prehner merged 2 commits into
itt-ustutt:masterfrom
dhfwc18:fix/value-in-with-celsius
Apr 28, 2026
Merged

fix: added branch for value_in to handle Celsius#108
prehner merged 2 commits into
itt-ustutt:masterfrom
dhfwc18:fix/value-in-with-celsius

Conversation

@dhfwc18
Copy link
Copy Markdown
Contributor

@dhfwc18 dhfwc18 commented Apr 22, 2026

Added branching logic to handle conversion to Celsius in the value_in method of PySIObject. Celsius now a valid input for the unit argument.

Honestly suprised I haven't caught this one given that I worked in low carbon heating. But the current implementation of value_in that I suggested few days ago does not take into account the seperate implementation of Celsius so results.value_in(si_units.Celsius) would return a TypeError because Celsius is not an instance of SIObject.

The best way I can think of to fix this is to add a branch to handle Celsius as you did with __truediv__.

Think the overhead this fix add should be negligible. Not sure what you guys think?

Added branching logic to handle conversion to `Celsius` in the
`value_in` method of `PySIObject`. `Celsius` now a valid input for the
`unit` argument.
@prehner
Copy link
Copy Markdown
Contributor

prehner commented Apr 23, 2026

Thanks for the fix, I didn't foresee this initially. While we're at it, can you also add the value_in method for Angle? Then we have everything consistent.

@prehner
Copy link
Copy Markdown
Contributor

prehner commented Apr 23, 2026

Sorry, maybe I was unclear. What I meant was basically

impl Angle {
    ...
    fn value_in(&self, unit: &Self) -> f64 {...}

Just to avoid confusion about the value_in function. Most users will not notice that Angle is actually a different class than SIObject, so this is more coherent that way.

`Angle` now mirrors `SIObject` with its own `value_in` method.
@dhfwc18
Copy link
Copy Markdown
Contributor Author

dhfwc18 commented Apr 23, 2026

Yes sorry I deleted my message

@dhfwc18
Copy link
Copy Markdown
Contributor Author

dhfwc18 commented Apr 23, 2026

I notice what you meant. Havent read up on Angle when I implemented the fix. Realised what you meant. Added the mirror. Hope this works!

@prehner prehner merged commit 584ef20 into itt-ustutt:master Apr 28, 2026
21 checks passed
@dhfwc18 dhfwc18 deleted the fix/value-in-with-celsius branch April 28, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants