-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
Various refactorings #1470
base: main
Are you sure you want to change the base?
Various refactorings #1470
Conversation
adamnemecek
commented
Jan 5, 2025
- made OMatrix::sum faster by using add_assign
- use map on optionals where possible
- refactored partial_cmp
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.
Thanks, overall these are nice style improvements. For future reference, they'd be a lot easier to review if you grouped changes of the same kind into the same commit, and force-pushed when you discover an error rather than fixing it in a later commit.
src/lib.rs
Outdated
} else { | ||
None | ||
} | ||
a.partial_cmp(b).map(|ord| if ord.is_ge() { b } else { a }) |
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.
To avoid a change in behavior, this should be is_gt
(and similar below)
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.
This is the correct behavior, is_ge
== matches!(self, Greater)
which is what was there before.
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.
That's incorrect. The "e" stands for "equals", as in "greater or equals". This is illustrated in the documentation.