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.

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

Added implicit conversion from Scalar to double #977

Closed

Conversation

cedricmoers
Copy link

  • Added implicit conversion from Scalar to double
  • Made exception for ScalarTestBase to ignore the Convert_ToBool_ThrowsInvalidCastException test for Scalar

- Added implicit conversion from Scalar to double
- Made exception for ScalarTestBase to ignore the Convert_ToBool_ThrowsInvalidCastException test for Scalar
@cedricmoers
Copy link
Author

cedricmoers commented Oct 14, 2021

I also wonder if the implicit conversion from double to Scalar should also be implemented in a similar way:

        /// <summary>Defines an implicit conversion of a <see cref="Scalar"/> to a <see cref="double"/>.</summary>
        public static implicit operator double(Scalar scalar)
        {
            return scalar.Amount;
        }

        /// <summary>Defines an implicit conversion of a <see cref="double"/> to a <see cref="Scalar"/>.</summary>
        public static implicit operator Scalar(double value)
        {
            return Scalar.FromAmount(value);
        }

Also, should this mean that all arithmetic operations should be based on Scalar instead of Double? For example, if you divide a voltage by a voltage, would you then rather return a 'double', or a Scalar?

@angularsen
Copy link
Owner

Also, should this mean that all arithmetic operations should be based on Scalar instead of Double?

I don't think so, do you have a use case for this where it would add value?

Scalar was initially introduced in #849 as a means to provide numeric values as an IQuantity, so in my mind it solves an edge-case. I would personally prefer to use double in most of my arithmetic.

@angularsen
Copy link
Owner

I also wonder if the implicit conversion from double to Scalar should also be implemented in a similar way:

Sure, add implicit cast both ways. I think that makes sense.

I have run into issues in other projects with implicit casts in certain scenarios. Can't recall the details, but I believe it was related to method overloads and it not always being intuitive what overload is chosen. I don't immediately recognize such issues here though, so let's just see I guess.

@cedricmoers
Copy link
Author

In my scenario I use (and love) UnitsNet for scientific measurements. I have some value processing blocks that are by nature 'unit independent'. One of these blocks is for example a value processor that returns an average value of multiple measurements:

    public interface IValueProcessor
    {
        IQuantity Get(Func<IQuantity> quantities);
    }

    public class Average
    {
        public int Count { get; set; }
        IQuantity Get(Func<IQuantity> quantityProvider)
        {
            int count = Count;
            IQuantity sum = null;
            for (int i = 0; i < count; i++)
            {
                var q = quantityProvider();
                if (sum is null)
                    sum = q;
                else
                    sum = (dynamic)sum + q;
            }

            IQuantity avg = (dynamic)sum / count;

            return avg;
        }
    } 

The scalar is a big improvement here, since before the Scalar, I had to create a separate class for processors taking quantities and processors taking int/double/decimal, ... Before this, I had to use 'object' as the common class and check if the object was an IQuantity or a double/int/decimal. This became a bit ugly and I created a 'wrapper': IQuantifiable and some helper Structs. I never was happy with that and Scalar is likely to solve that.

Currently I see one (edge-)usecase that would benefit of returning the Scalar from arithmetic operations. Imagine I have a processor that does a Division on a given quantity that I do not know the type of:

    public class Divider
    {
        public IQuantity DivideBy { get; set; }
        IQuantity Get(Func<IQuantity> quantityProvider)
        {
            var q = quantityProvider();
            return (dynamic)q / DivideBy;
        }
    }

If the measured quantity is 5V and I want it divided by 2V, then the result I expect is 2.5 (no unit). Unfortunately, because the division in the ElectricPotential struct returns a 'double', which is not a Quantity, and thus the above will throw an InvalidCastException. If the division would return a Scalar, the above (I think) would work fine.

If the expected result is in fact a double, then the implicit cast will do the correct casting for you.

Thus I see an advantage in returning a Scalar from arithmetics when normally a 'double' is returned, but I cannot oversee what the impact will be and how that will break the current system. I am willing to try this out.

@vanBroekhoven
Copy link

This pull request is the best idea I've seen in a while :D

@angularsen
Copy link
Owner

angularsen commented Oct 16, 2021

Thank you for the use case of returning Scalar in arithmetic, it helped my understanding.

I guess it does provide value if trying to keep everything IQuantity. I have not used the library like that myself so it is foreign to me, but it does seem useful.

I guess, if we can change all arithmetic method overloads to return Scalar, and provide implicit casts to double, then hopefully 🤞 all existing tests and people's application code will compile and run exactly as before?

If we get tons of compile errors in tests with this change, then that would be a breaking change and something for #180 .

I propose you go ahead and try, and see how it works out.

On a side-note, it seems .net6 might bring some generic math too. Maybe that will help in similar scenarios.
https://devblogs.microsoft.com/dotnet/preview-features-in-net-6-generic-math/#generic-math

@cedricmoers
Copy link
Author

Interesting. Arithmetics in interfaces is something I have been waiting for for a while.
I have tried to use Scalars for the arithmetics and didn't stumble upon major issues, at least not in the unit tests. There was one test that had an ambiguity error as you noted before. I will continue to investigate for the best non-intrusive outcome.

I do expect that some use cases will break in some scenarios, altough I'm not sure. In my case, when the result of a function could be a Quantity, or a double, I had to return an object since that seemed like the best option before I knew about Scalar. If then there is some code that checks for the type of that object, and executes some code accordingly, well that outcome could go differently then. Just something to keep in mind if we decide to go the Scalar way.

@angularsen
Copy link
Owner

@cedricmoers Some more comments on Generic Math.
#780 (comment)

@stale
Copy link

stale bot commented Jan 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 3, 2022
@angularsen
Copy link
Owner

I'm sorry, what is the status of this PR again?

It seems my last suggestion was

I guess, if we can change all arithmetic method overloads to return Scalar, and provide implicit casts to double, then hopefully 🤞 all existing tests and people's application code will compile and run exactly as before?

Should we try changing all arithmetic methods retuning double to return Scalar instead? Or should we merge this PR as-is?

@stale stale bot removed the wontfix label Jan 3, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 16, 2022
@stale stale bot closed this Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants