Ohm's Law Overloads for ElectricPotentialAc & ElectricPotentialDc

[McNeight]

Describe the bug Per Ohm's Law, multiplying an ElectricPotential object with an ElectricCurrent object results in a Power object. Attempting to use an ElectricPotentialAc or ElectricPotentialDc object results in error CS0019.

To Reproduce Steps to reproduce the behavior:

1. var x = new ElectricPotentialDc(5, UnitsNet.Units.ElectricPotentialDcUnit.VoltDc) * new ElectricCurrent(2, UnitsNet.Units.ElectricCurrentUnit.Ampere);
2. See error: CS0019

Expected behavior x is a Power object of 10 Watts.

Additional context Perhaps I'm not understanding the purpose of the Ac and Dc objects and their difference from the "basic" ElectricPotential unit? I'm not sure if adding more operators to cover the Ac and Dc classes, or possibly having them inherit the operator from a base class is the better approach.

[lamarch]

I think that you should go with ElectricPotential instead, because it properly implements Ohm's law.

As we can see, theses special AC/DC units were added in commit f82e49e . However, looking in UnitsNet/CustomCode/Quantities, no multiplication behavior have been implemented, for both ElectricalPotentialAc and ElectricalPotentialDc.

Frst, I think that ElectricalPotentialDc is redundant with ElectricalPotential, which implements the correct multiplication operations with current.

Moreover, we can see that the same commit added ReactivePower and ApparentPower, and if I can correctly remember my electrical engineering course, reactive power and apparent power are calculated using complex current and complex potential (that uses the frequency of the quantity as well as its phase shift). I honestly think that theses very special quantities are beyond the scope of this API. Another thing catches my curiosity here : ElectricalCurrentAc and ElectricalCurrentDc are both missing, which is inconsistency.

The direct way to solve this misunderstanding would be to remove theses 2 quantities : ElectricalPotentialDc & ElectricalPotentialAc. I don't think that ReactivePower & ApparentPower should be removed, that being said. Considering that it would be a breaking change, I propose to mark them as obsolete, and to redirect on ElectricalPotential.

We could also implement complex quantities, using complex numbers, for AC current and potential, which would partially solve this issue, however, we would still need to include the frequency parameter with theses quantities. Outrageous complexity...

[angularsen]

I have no strong opinion here, but a few thoughts:

• Both AC and DC are probably useful to represent, but I'm not sure we need separate quantities and units for them?
• In my mind, AC is the "odd" one and has a more narrow usage in electronics
• ElectricPotential is based on https://en.wikipedia.org/wiki/Electric_potential and has more broad use in physics, but in my mind it can be used to represent DC voltage.

Proposal:

• As @lamarch suggested, obsolete/remove ElectricPotentialDc and ElectricPotentialAc, use ElectricPotential instead.
• Someone™ can create wrapper types to help work with AC and DC domain, to represent additional information and provide useful methods. For example:
  • AlternatingCurrent with ElectricCurrent + Frequency
  • AcVoltage with ElectricPotential + Frequency
  • Create methods for these to calculate AC related stuff, like power, root mean square voltage and whatnot: https://en.wikipedia.org/wiki/Alternating_current#Mathematics_of_AC_voltages

Not sure if DC benefits from such wrapper types or if it is already covered by ElectricPotential, ElectricCurrent etc.

As always, it is best for someone actually working in this domain to help decide what is a good design for UnitsNet. I can help making the changes fit in UnitsNet.

[McNeight]

@lamarch @angularsen

With #1312 I've put together a straw man for both removing the ElectricPotentialAc and ElectricPotentialDc types, as well as creating a wrapper type for ElectricCurrent (for starters).

My main concern at this point is getting the terminology correct. For example, ElectricCurrent as alternating current measured as root-mean square is supposed to be equivalent to a direct current when applied to a resistive load. However, that doesn't make them identical, although my current "implementation" treats them as such.

It's trivial to calculate the peak current and peak-to-peak current from there, but then what happens when an AC ElectricCurrent has a DC ElectricCurrent added to it? In reality, the RMS and peak would change because of the resulting bias offset, but the peak-to-peak (distance between peaks) would remain the same?

I'm not sure if I'm capturing too much information, or not enough information, to enable these kinds of transforms.

[angularsen]

I need some time to get around to this and understand the proposal.

[angularsen]

I posted an initial review of #1312 , let's continue the discussion there.

[McNeight]

So I've spent some time thinking about this, and to be honest, I think the best answer is to remove both ElectricalAc and ElectricalDc, have just Electrical* quantities, and tell anyone who wants to map more functionality into it (volts RMS, phase angle, etc.) to just use a named tuple.

That allows the implementer to contain as much or as little information as they would like, while using multiple quantities available in Units.Net.

[lamarch]

@McNeight I totally agree with you.

We can see these Electrical quantities as instant measurements, so they are not concerned by the frequency and phase shift. If someone wants to take into account that a current is alternative, it is best to use other variables for theses times quantities, and let them design their power calculation.

If we really want to implement such power calculation, in addition to frequency and phase shift, we'll also need to consider the waveform (sine, triangular, square wave)... Which I think is "off topic" for this library.

[angularsen]

Thanks for the input guys, closing for now then.

[angularsen]

I think the best answer is to remove both Electrical_Ac and Electrical_Dc, have just Electrical* quantities, and tell anyone who wants to map more functionality into it (volts RMS, phase angle, etc.) to just use a named tuple.

Is anyone up for creating a pull request against release/v6 branch, to make these changes?

XMLDoc for the quantities should have remarks explaining what you said above, that advanced functionality like RMS, phase angle etc should be handled outside the library.