Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Conversion to/from complex storage types is wrong #452

Open
mkalte666 opened this issue Jan 15, 2024 · 0 comments · May be fixed by #453
Open

Conversion to/from complex storage types is wrong #452

mkalte666 opened this issue Jan 15, 2024 · 0 comments · May be fixed by #453

Comments

@mkalte666
Copy link

mkalte666 commented Jan 15, 2024 •
edited
Loading

Heya, i have run into the following issue:

Lets assume a complex impedance like Z = (123 + 321j) * ohm

use uom::fmt::DisplayStyle;
use uom::si::complex64::ElectricalResistance;
use uom::si::electrical_resistance::ohm;
use uom::num_complex::Complex64;

fn main() {
    let n = Complex64::new(123.0,321.0);
    let unit = ElectricalResistance::new::<ohm>(n);
    println!("{} -> {}", n, unit.into_format_args(ohm,DisplayStyle::Abbreviation));
    assert_eq!(n,unit.get::<ohm>());
}

this fails:

123+321i -> 343.7586362551492+0i Ω
thread 'main' panicked at src\main.rs:10:5:
assertion `left == right` failed
  left: Complex { re: 123.0, im: 321.0 }
 right: Complex { re: 343.7586362551492, im: 0.0 }

343.7586362551492 being the norm of the number.

I have looked at the implementing PR: while i would expect the pure unit conversion factor to be a floating point type, the actual conversion operation must be in the complex numbers, otherwise the operation is one-way (there are infinite solutions to |z|^2 = re^2 + im^2 for a single given z; think vectors describing a circle of radius z).

Im gonna have a go at either understanding where im wrong in my assumptions, or fixing this, as i really want complex impedance :D
@mkalte666 mkalte666 changed the title Conversion to/from complex storage types is weong Conversion to/from complex storage types is wrong Jan 15, 2024
mkalte666 added a commit to mkalte666/uom that referenced this issue Jan 15, 2024
@mkalte666
Conversion now has both a value and a factor type
b285dc8 
Initially, `ConversionFactor` and thus `Conversion::T` requred `PartialEq`.
This makes sense for the conversion factor itself (i.e. scaling across
units), however it breaks once you introduce complex numbers.

Those can *still* be scaled just like normal numbers - you essentially
just increase or decrese a vector length, but the conversion function
cannot compare them - "Z_1 < Z_2" is not trivially decidable.

It is, however, also not needed - unit scales are just that - scalars
that scale. And those can be easily compared.

This commit seperates `Conversion::T` into `Conversion::VT` and
`Conversion::T` and moves the `PartialEq` requirements from
`ConversionFactor` into `Conversion::TT` directly.

This requires a lot of trait bounds added down the line, so im not 100%
that this does not break anything down the line.

There might be a nicer way to go about this, but i haven't found any.

closes iliekturtles#452
@mkalte666 mkalte666 linked a pull request Jan 15, 2024 that will close this issue
Draft
mkalte666 added a commit to mkalte666/uom that referenced this issue Oct 14, 2024
@mkalte666
Conversion now has both a value and a factor type
8d36211 
Initially, `ConversionFactor` and thus `Conversion::T` requred `PartialEq`.
This makes sense for the conversion factor itself (i.e. scaling across
units), however it breaks once you introduce complex numbers.

Those can *still* be scaled just like normal numbers - you essentially
just increase or decrese a vector length, but the conversion function
cannot compare them - "Z_1 < Z_2" is not trivially decidable.

It is, however, also not needed - unit scales are just that - scalars
that scale. And those can be easily compared.

This commit seperates `Conversion::T` into `Conversion::VT` and
`Conversion::T` and moves the `PartialEq` requirements from
`ConversionFactor` into `Conversion::TT` directly.

This requires a lot of trait bounds added down the line, so im not 100%
that this does not break anything down the line.

There might be a nicer way to go about this, but i haven't found any.

closes iliekturtles#452
mkalte666 added a commit to mkalte666/uom that referenced this issue Oct 14, 2024
@mkalte666
Conversion now has both a value and a factor type
2a8261a 
Initially, `ConversionFactor` and thus `Conversion::T` requred `PartialEq`.
This makes sense for the conversion factor itself (i.e. scaling across
units), however it breaks once you introduce complex numbers.

Those can *still* be scaled just like normal numbers - you essentially
just increase or decrese a vector length, but the conversion function
cannot compare them - "Z_1 < Z_2" is not trivially decidable.

It is, however, also not needed - unit scales are just that - scalars
that scale. And those can be easily compared.

This commit seperates `Conversion::T` into `Conversion::VT` and
`Conversion::T` and moves the `PartialEq` requirements from
`ConversionFactor` into `Conversion::TT` directly.

This requires a lot of trait bounds added down the line, so im not 100%
that this does not break anything down the line.

There might be a nicer way to go about this, but i haven't found any.

closes iliekturtles#452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Conversion now has both a value and a factor type mkalte666/uom
1 participant
@mkalte666