Skip to content

Support NaN #176

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

Closed
sliekens opened this issue Aug 2, 2016 · 6 comments
Closed

Support NaN #176

sliekens opened this issue Aug 2, 2016 · 6 comments

Comments

@sliekens
Copy link

sliekens commented Aug 2, 2016

Please add NaN properties for every unit.

For example

  • Energy.NaN
  • Speed.NaN

Also add IsNaN(unit)

  • Energy.IsNaN(Energy.NaN): true
  • Energy.IsNaN(Energy.MaxValue): false

Thanks!

@angularsen
Copy link
Owner

angularsen commented Aug 2, 2016

We could, but I question the value of adding this.

double.IsNaN(Length.FromMeters(double.NaN).Centimeters) returns true, as expected.
Is this a matter of syntactic convenience?

I also notice that TimeSpan.FromSeconds(double.NaN) throws ArgumentException

TimeSpan does not accept floating point Not-a-Number values

Not sure what is the best practice here. Personally I might prefer throwing exception, not a fan of NaN bugs that are unnecessarily harder to debug. I very very rarely use NaN for anything useful.

@sliekens
Copy link
Author

sliekens commented Aug 9, 2016

Is this a matter of syntactic convenience?

Yes. :)

I also notice that TimeSpan.FromSeconds(double.NaN) throws ArgumentException

I guess because TimeSpan stores its state as Int64 (number of ticks), and there is no Int64.NaN.

@angularsen
Copy link
Owner

@eriove @bretthysuik @maherkassim @tongbong

You are one of the top contributors, any input on NaN values?
a) Allow NaN values, can check double.IsNaN(myLength.Meters) (current behavior)
b) Allow NaN values, add IsNaN getter as syntactic convenience (suggested by @StevenLiekens)
c) Throw exception on trying to construct with NaN value, similar to how TimeSpan works (suggested by me, breaking change)

@bretthysuik
Copy link
Contributor

My vote would be c) and to not re-implement .NET BCL NaN detection functionality in this library. double.IsNaN(myLength.Meters) is simple and clean enough to me without having to expand the API of this library.

@eriove
Copy link
Contributor

eriove commented Aug 17, 2016

Sorry for the late reply. Read the e-mail in my phone and forgot to reply.

c) is my favorite, since it catches errors early and NaN is usually an error when it comes to physical measures. Since it breaks backwards compatibility my second choice would be b. But I can't say I really care that much when choosing between a and b.

@angularsen
Copy link
Owner

It seems you both share my opinion on generally not facilitating the use of NaN. I'm adding a future reminder in #180 to consider option c) if we are going to bump the version later.

Seeing that you are both in favor of c), I think doing b) would just cause yet more future breaking changes, so that leaves a). Do nothing. The simplest solution to most problems :-)

Thanks for the input guys.

@angularsen angularsen mentioned this issue Sep 27, 2018
25 tasks
angularsen added a commit that referenced this issue Dec 16, 2018
# 4.0.0 Release
This PR will serve as the list of items to complete and it will be updated to show the progress before finally merged into `master` when completed. We all have busy schedules, so **if you want to help move this work forward then that is much appreciated!** 

## The main theme is to reduce binary size
In two years it has grown from 280 kB to 1.4 MB and a lot of it is due to unnecessary syntactic sugar with many method overloads for various number types and nullable types - for every of our 800+ units! It simply adds up to a big total.

These items are chosen from #180, trying to include as many of the low hanging fruits as possible, while still keeping the list short and realistic to complete in a reasonably short time - we are all busy in our daily lives. We can always have more major version bumps later than trying to perfect it all now.

## Feature complete before November, merged before mid-December
I would like to aim for a "beta" pre-release nuget sometime in October with all the items completed, so we can have some time to test it before releasing the final, stable 4.0.0 version before Christmas holidays.
We should have the work items list more or less final before Monday, October 8th.

## Changes
#### Added
- [x] UnitSystem with a different meaning, now defines the base units for a unit system (#524)

#### Removed
- [x] Replace netstandard1.0 target with netstandard2.0, to avoid the extra dependencies (#477)
- [x] Remove code marked as `[Obsolete]`, such as `VolumeUnit.Teaspoon`  (#490)
- [x] Remove nullable `From` factory methods (#483)
- [x] Remove extension methods on _nullable_ number types (#483)
- [x] Remove all number extension methods (#497)
- [x] Remove `Length2d`, replaced by `Area` (#501)
- [x] Remove static methods on `UnitSystem`, should use `UnitSystem.Default` instead (#496)
- [x] Remove unit parameter from `ToString()` methods (#546)

#### Changed
- [x] Throw exception on NaN values in static constructor methods, like `FromMeters()` (#502, see #176 (comment))
- [x] Throw if unit was not specified when constructing quantities, (#499 - #389 (comment))
- [x] Stricter parsing (#343 and #180 (comment))
- [x] Remove unit parameter from `ToString()` methods (#546)
- [x] Change Temperature arithmetic back to use base unit Kelvin (#550, see #518)

#### Renamed
- [x] Correct SingularName for some Flow unit definitions (#494, see #360)
- [x] Split/rename UnitSystem into UnitParser, GlobalConfiguration, UnitAbbreviationsCache (#511)

#### Fixed
- [x] Search for any TODO comments in the code to address, remove comment and either fix or create issue (5d24432)
- [x] Update README with v4 changes #498
- [x] Do not try/catch in UnitConverter.Try-methods (#506)
- [x] Do not try/catch in `Length.TryParse()` and for other quantities (#507)
- [x] Add/update changelog in wiki for v4 with some summary copied from this issue, some v3 to v4 migration instructions and briefly explain why some stuff were removed

## Milestones
- [x] Finalize work items list (Monday, October 8)
- [x] Release 4.0.0-beta1, feature complete (Wednesday, October 31)
- [ ] ~Show release notes and upgrade guide when upgrading to 4.x nuget, if possible~
- [x] Release 4.0.0 (Monday, December 17)
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

4 participants