MadsKirkFoged / EngineeringUnits

Working with units made easy with automatic unit-check and converting between units
MIT License
42 stars 10 forks source link

concept for simplifying algebraic units #4

Closed jccockre closed 3 years ago

jccockre commented 3 years ago

In my application there is a strong desire to present user-friendly string values when calling ToString().

Currently when multiplying, say, ElectricPotential times ElectricCurrent the result can be read in Watts, but only if hard-coded as Power. If the result is variable or dynamic, the units will read kg * m2 / s3 which is not ideal from the customer's standpoint.

Introducing a simplification method like I've demonstrated here allows automatically casting to named, human-readable units.

Note: This does not work for Frequency because the library seems to consider the base unit of Frequency to be 1 / s. I can work around that if necessary. But my personal preference would be that Hertz become the base unit of Frequency under the principle that we should not use algebraic expressions when representing units if there exists a well-recognized English word for the same unit.

jccockre commented 3 years ago

Don't merge this yet. If you like the concept I will try to populate it with all of the units.

MadsKirkFoged commented 3 years ago

@jccockre, I'll try to retell what I think you want. Let me know if im not correct.

            SpecificEntropy P1 = new SpecificEntropy(1, SpecificEntropyUnit.JoulePerKilogramKelvin);
            MassFlow M1 = new MassFlow(1, MassFlowUnit.KilogramPerSecond);
            Temperature T2 = new Temperature(10, TemperatureUnit.DegreeCelsius);
            Temperature T1 = new Temperature(5, TemperatureUnit.DegreeCelsius);

            Power Q1 = M1 * P1 * (T2 - T1);

            Debug.Print($"{Q1}"); //5 W
            Debug.Print($"{M1 * P1 * (T2 - T1)}"); //5 m²kg/s³

When you have an equation but you dont tell the resulting unit, you still want to show the same unit. Like the above example, you want both to show 5 W?

My first thought, is that when you dont 'hardcode' the Power Q1 = you no longer have a unit-check, which is the whole point of this library.

My second thought is that it could be done but you will have to create a massive if-block where you check against every unit we have.

Could it be done so all this code will only be called if you .ToString() a UnknownUnit? This will only slow down the users actually using it

jccockre commented 3 years ago

Great points. You understood my intentions correctly. I have updated the pull request:

Do you have any opinion on writing 1 / s as Hertz by default? It can be forced through type-casting but my opinion is that English names should be preferred over algebraic expressions in the ToString() by default.

MadsKirkFoged commented 3 years ago

The extension method returns BaseUnit rather than dynamic It is only called inside ToString() on an UnknownUnit

Approved! Good work!

Personally I like Hertz better then 1 / s

The problem with the massive if-block is not necessary creating it the first time, but that future users need to remember to add new unit to it. Could you use this code (or something similar) that gets every child-class of BaseUnit? And maybe only handle special cases like Hertz as if-cases

            IEnumerable<BaseUnit> exporters = typeof(BaseUnit)
            .Assembly.GetTypes()
            .Where(t => t.IsSubclassOf(typeof(BaseUnit)))
            .Select(t => (BaseUnit)Activator.CreateInstance(t));

            foreach (BaseUnit item in exporters)
            {
                //Print name of unit
                Console.WriteLine(item.GetType());
            }
jccockre commented 3 years ago

Yes I agree. However we do not want to do all of that reflection at run-time, as it will be a big performance hit.

This took quite a bit of tinkering but I believe I got it working. The build order is:

  1. Build EngineeringUnits (with a do-nothing stub for IntelligentCast)
  2. Build CodeGen
  3. Execute CodeGen.exe
  4. Build EngineeringUnits (now with a populated IntelligentCast method body)
  5. Build UnitTests (important that this occurs last so tests will pass)

There is no need to take special action as a user. I configured the post-build event property in CodeGen to ensure the code gets generated.

However, please note a significant defect in my commits: I hard-coded the absolute path to MSBuild.exe to get the build working on Azure. It seems that simply the command msbuild is not found in the PATH variable, and I am not clever enough to understand how to get the executable path on-the-fly. If you have any insight to offer there, please let me know. In the mean-time I guess this build will work as long as the Azure instance has Visual Studio 2019 Enterprise installed.

I still have not figured out how to get 1 / s to read as Hertz by default. Curious if you have any advice there.

MadsKirkFoged commented 3 years ago

Great work @jccockre!

I'm a bit worried about the autogenerated code (mostly because I have never worked with it before) but that is all on me.

Could we maybe for a start just manually run the CodeGen and let it generate a file that is just stored locally in the project? That way we dont have to mess with Azure Would that be fine with you?

I looked into the Hertz case - its just a mistake from my side! I had set its SI unit (default) to 1/s instead of Hz https://en.wikipedia.org/wiki/Frequency ----> SI unit: hertz (Hz)

I have fixed that so it should show Hz as default now

jccockre commented 3 years ago

OK I have updated the PR with the following changes:

We need to remember this command and keep it somewhere safe: "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\msbuild.exe" "$(SolutionDir)EngineeringUnits/EngineeringUnits.csproj" /p:PreBuildEvent="$(TargetDir)CodeGen.exe"

If one of us ever learns enough about Azure to be able to invoke msbuild without the absolute path, we should circle back to this.

I really appreciate you fixing the Hertz issue! All of my test cases are passing now. This is going to be big for my customers because my application uses Frequency probably more than any other units.

MadsKirkFoged commented 3 years ago

I agree with Azure! You are welcome - Is it something public you are creating? i'm just curious to see how other people are using this library

Is it ready for me to merge it?

jccockre commented 3 years ago

Yes, I think this one is ready to merge.

I'm starting the README for my new library now. I will let you know as soon as it is presentable.