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

Exception in Math.Pow #642

Closed
Ellerbach opened this issue Sep 13, 2020 · 7 comments
Closed

Exception in Math.Pow #642

Ellerbach opened this issue Sep 13, 2020 · 7 comments

Comments

@Ellerbach
Copy link
Member

Details about Problem

nanoFramework area: (C# code)

Target: EXP32_VROOM_32

Description

Math.Pow(1.01580092094650000000000000, 0.19029495718363400000000000000)

Using the latest System.Math version 1.3.1-preview.2

Raises an exception 'Exception thrown: 'System.NotImplementedException' in System.Math.dll'

Make an effort to fix the bug

As it's a NotImplementedException, I checked the System.Math and it's calling an external function. Looking at the rest, didn't really find it in the ESP32 specific code. Still trying to understand the full structure.

@josesimoes
Copy link
Member

This is expected. On nF we have the Math API available with float and double parameters.

See the remarks here and a detailed explanation here.

Some numbers supporting the decision can be found here: #426 too.

If you're using the stock image it does NOT have support for DP floating point so the code above should be replaced with:
Math.Pow(1.0158009209465f, 0.190294957183634f)

@Ellerbach
Copy link
Member Author

I see. Thanks. Will cast the double into floats. For non supported platforms, would it be ok to have automatic cast into the (double, double) function? I know that would make not as good precision but still would make it more transparent for code.

@josesimoes
Copy link
Member

Back then we had a lengthily discussion about that... 😁
Was around that "simply" casting the double parameters to floats internally would be hiding that the platform wasn't dealing properly the types advertised. And that could be cheating or, at best, misleading.

The solution was to add those overloaded methods with float types and throw NotImplementedException with the explicit intent to caught the developer attention to this fact and have him decide the best course of action according to it's requirements.
Hopefully this would make it abundantly clear how this is being handled in the platform.

Which, from our conversation, seems to be working as planned. 😉

As a side comment: your code above, technically, seems to be using floats anyway as the "extra" precision figures are zeroed...

@Ellerbach
Copy link
Member Author

Yes code above using float because it's the results of some maths. Done with double on the nF side. Which then, I understand why now, are the result of float calculation.
I understand the rational and the discussion. Now, I'm looking at the best path to reuse with minimum transformation code from .NET IoT and offer lots of sensors on nanoFramework. And as we initially, after long discussions as well, decided to go for double, then, all the helpers, and all the internal are double... Reality is that this precision is never needed. But we have to think of this for the migration path and it adds to the list. An intermediate Math. code can be used to determine if there is a natif DF support or not and under the hook, cast or not.

@josesimoes
Copy link
Member

Glad that you've pointed that out because I forgot to mention it. The details on the implementation available on the device running it are exposed with this

Guess we can "move" that property to System.Math to make it more readily available.

Or that we can simply cast double to float if the platform requires it... but, then again, that would be "hiding" the real handling...

I perfectly understand the need to make this as simple and efficient as possible and we are, of course, willing to incorporate any necessary adjustments.

Let me give this some thought and please keep the matter open. I'm sure we'll reach a satisfactory outcome.

@Ellerbach
Copy link
Member Author

Thanks for the link. Few things to keep in mind: if you make double available as a native struct while it's not supported on the native platform, it's kind of misleading anyway 😉 so why not casting in all the math functions? At least it's coherent.
If you take my example, I have double everywhere. Under the hook, you have float. If I call a function with in System.Math with double, I get an exception.
So either, you don't allow to use double at all on those platform and it's consistent up to the end, either you should allow to have it everywhere even if under the hook it's casted to float.
Happy to discuss it further.

@josesimoes
Copy link
Member

I understand your point.

To be honest, IMHO, this is more a technicality and not a real issue.
Derives from the pickiness of wanting to be strict and trying mirror .NET double type with the C/C++ double type.
But this tries to be a democratic project... 😉

The path of not providing the double .NET type would be kind of insane: requires another mscorlib, which in turn, requires another version of all other class libs (that ref it) and also requires (as it is now) the option to build the firmware with or without support for DP FP. That clearly is not worth the effort not to mention the mess and potential confusion among developers just to try understanding it. 😅

A reasonable approach would be something like: drop the float Math API, cast to float at native code (if needed), keep that property advertising the native implementation of the platform that's running (in case that's interesting/relevant for the code using it). The build option is perfectly valid and should remain to allow building it with more or less precision as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants