-
Notifications
You must be signed in to change notification settings - Fork 15
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
Operations with unreduced fractions #65
Merged
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
e9d6c46
operations with unreduced fractions should now produce unreduced frac…
lipchev 3c2ded8
replicated the BasicMath bencharks using their non-reduced counterparts
lipchev 6093307
added short-path testing for-1 and 0 to ReduceTerms
lipchev ed31261
-one check too many: 0 % 10 is 10
lipchev c71d513
- fixed the issues with ToDecimalWithTrailingZeros
lipchev 65f1ca0
added support for creating non-reduced fractions when parsing a decim…
lipchev 9d5a5c2
Replace methods with default parameters with two methods to remain AB…
danm-de 1b196ba
- Added missing FromString overload.
danm-de 2069684
Corrected unit test for JsonFractionConverter (breaking change)
danm-de 2bd6d07
Move FromString method to a separate file: Fraction.ConvertFromString…
danm-de f3cbf8f
Move FromDouble method to a separate file: Fraction.ConvertFromDouble…
danm-de c708381
Move FromDecimal method to a separate file: Fraction.ConvertFromDecim…
danm-de a9e4ea4
Fix nullability (FromString) + compiler warnings in FractionTypeConve…
danm-de ab41e0e
Only one ternary conditional operator in a (single) statement.
danm-de 870af26
Cleanup code - InvalidNumberException is not longer used.
danm-de 2d81d10
- Cleanup xmldocs
danm-de a04dfff
Fix XML-doc comments
danm-de 07bef43
Code cleanup - remove compiler warnings
danm-de 54b390b
Fix XML-doc comments
danm-de ffda4fe
update documentation
danm-de 21d7ab2
Fix readme (terms are reduced when using multiplication)
danm-de 291c28e
Added line breaks to avoid a single line displayed on github.com.
danm-de a1f4aa2
Try Mathjax new line symbol to insert line breaks.
danm-de f52d1a4
Another try to introduce line breaks in a MathJax formular (latex)
danm-de ba15cef
Added 2 spaces in order to get a linebreak (please?)
danm-de File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no particular preference, but have you considered actually making the Equality/HashCode based on reduced terms? Given that
123.00m
is considered equal to123m
I don't think it would be inconsistent to say that12300/100
is equal to123/1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot estimate the performance impact on existing code. Currently, most users are likely to use normalized fractions anyway, then the following applies:
If
State == FractionState.IsNormalized
or_normalizationNotApplied == false
is set for both fractions, then the numerator and denominator can simply be compared.However, the situation is different with non-normalized fractions. The expensive method
Reduce
would have to be called every time for bothGetHashCode
andEquals
. And we cannot cache the results (readonly struct).Another question would be: What if for a use case the following applies:$\frac{1}{2} \neq \frac{2}{4}$ (because these are different representations in the user interface)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had a long discussion with my colleagues back then. The majority believed that
Equals
should always be exact. Microsoft does not follow this rule. In our software we have a problem with this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Equals is
true
then the HashCode should be equal as well, the opposite is not a given.The GetHashCode is mostly used when checking for stuff like
collection.Contains(x)
(where collection is actually hash-based)- otherwise we hit the Equals check, which for our scenario can be quite efficient. For actual GetHashCode- related checks, well as you said- it's more likely that somebody is doing the something custom, and can override the way the hash-code is calculated.The unintentional / unexpected non-equality is what worries me a little- but we'll be fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the
.Dump()
output in no particular order, but of course:Equals(a,b) == true
impliedGetHashCode(a) == GetHashCode(b)
. In the example we have two different time zones - why are both values the same? If I want to know if it is the same point in time, I would apply.ToUniversalTime()
to both variables and then compare.But hey, I'm open to everything. You said you wanted to use the data type in another project (https://github.com/angularsen/UnitsNet right?). What is the opinion there? Is my imagined “use case” perhaps nonsense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we've had lots of issues with Equality contract and how to make it so that "1 kg" == "1000 g". My personal feeling is that making it custom is way easier then making it work for everybody.. I asked the AI some time ago about a design pattern to justify an argument- it cited the principle of least surprise which I quite liked. So as soon as we open the door the optimized operations someone (new) might get surprised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POLS, I remember it. Geoffrey James has been quoted quite a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me sleep and think about it one night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started: https://github.com/danm-de/Fractions/tree/feature/default-to-value-equality
Having fun because of NaN != NaN 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't know whoever thought it was necessary to have the Equals method differ from the == on this one 🤔