-
Notifications
You must be signed in to change notification settings - Fork 1
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
Squash commits to prepare for v2 #14
Merged
Merged
Conversation
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
The main change in the API is that functions in `IsApprox` with the same name as those in Base and `LinearAlgebra` are now different methods of the same function. So you can do `using` of both `LinearAlgebra` and `IsApprox` withouth qualifying any symbols. Another API change making the signatures uniform, rather than having the same argument in different places in different functions or methods. Making this change was enabled by the previous item. * A few details * Change argument order for Base.isapprox (#13) Previously, I had `Base.isapprox(::AbstractApprox, x, y)`. Now we use `Base.isapprox(x, y, ::AbstractApprox)`. This order agrees with all other testing functions included in this package, such as, `isone`, `isreal`, `ishermitian`, etc. * All predicates either extend `Base`/`LinearAlgebra` or are new to this package Previously, some predicates were copies of functions in `Base`/`LinearAlgebra`, but did not extend these functions with new methods, but rather were new functions under `IsApprox`. Other predicates in `IsApprox` had no function in `Base`/`LinearAlgebra` of the same name. This commit changes this so that all predicate functions of the same name as those in `Base`/`LinearAlgebra` now extend those functions. All predicate functions, including those extended from `LinearAlgebra` are exported. With this commit, you can do `using IsApprox` and suffer no name collisions with `Base`/`LinearAlgebra`. `JET` and `Aqua` find no ambiguities or piracy. * This commit and the previous, precursor commit, include a breaking change to the API. Methods like `Base.isapprox(::AbstractApprox, x, y)` are now `Base.isapprox(x, y, ::AbstractApprox)` Methods like `Base.isapprox(x, y; approx::AbstractApprox)` are now `Base.isapprox(x, y, ::AbstractApprox)` In other words, the calling convention is now uniform. * Other changes * Add tests and fix small bug * Use 42.0 rather than 42 in isinteger test A method for rationalize(::Int) does not exist in older versions (at leat 1.6) of Julia. So we use a float instead so that the test runs on more versions. * Fix bug in isunitary for Equal and IsApprox Don't compare result to zero. Add one and compare to one. This is the proper scale because we expect the vectors to be normalized. * Explain new API * Fix some incorrect statements in comments `Base.isapprox` does not use matrix norms. It treats multidimensional arrays as vectors. Closes #12 * Try updating codecov action Added repository secret to GH repo config as well. * Disable test using feature not in v1.0 * Add tests, remove two nonsense methods Looks like I tried to add methods to satisfy JET or Aqua in the past. But they don't have arg combinations that could ever be called, even because of ambiguity. I removed them and all tests, including JET and Aqua still pass.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
===========================================
+ Coverage 54.50% 94.77% +40.27%
===========================================
Files 3 5 +2
Lines 200 249 +49
===========================================
+ Hits 109 236 +127
+ Misses 91 13 -78 ☔ View full report in Codecov by Sentry. |
jlapeyre
added a commit
that referenced
this pull request
Oct 4, 2024
The main change in the API is that functions in `IsApprox` with the same name as those in Base and `LinearAlgebra` are now different methods of the same function. So you can do `using` of both `LinearAlgebra` and `IsApprox` withouth qualifying any symbols. Another API change making the signatures uniform, rather than having the same argument in different places in different functions or methods. Making this change was enabled by the previous item. * A few details * Change argument order for Base.isapprox (#13) Previously, I had `Base.isapprox(::AbstractApprox, x, y)`. Now we use `Base.isapprox(x, y, ::AbstractApprox)`. This order agrees with all other testing functions included in this package, such as, `isone`, `isreal`, `ishermitian`, etc. * All predicates either extend `Base`/`LinearAlgebra` or are new to this package Previously, some predicates were copies of functions in `Base`/`LinearAlgebra`, but did not extend these functions with new methods, but rather were new functions under `IsApprox`. Other predicates in `IsApprox` had no function in `Base`/`LinearAlgebra` of the same name. This commit changes this so that all predicate functions of the same name as those in `Base`/`LinearAlgebra` now extend those functions. All predicate functions, including those extended from `LinearAlgebra` are exported. With this commit, you can do `using IsApprox` and suffer no name collisions with `Base`/`LinearAlgebra`. `JET` and `Aqua` find no ambiguities or piracy. * This commit and the previous, precursor commit, include a breaking change to the API. Methods like `Base.isapprox(::AbstractApprox, x, y)` are now `Base.isapprox(x, y, ::AbstractApprox)` Methods like `Base.isapprox(x, y; approx::AbstractApprox)` are now `Base.isapprox(x, y, ::AbstractApprox)` In other words, the calling convention is now uniform. * Other changes * Add tests and fix small bug * Use 42.0 rather than 42 in isinteger test A method for rationalize(::Int) does not exist in older versions (at leat 1.6) of Julia. So we use a float instead so that the test runs on more versions. * Fix bug in isunitary for Equal and IsApprox Don't compare result to zero. Add one and compare to one. This is the proper scale because we expect the vectors to be normalized. * Explain new API * Fix some incorrect statements in comments `Base.isapprox` does not use matrix norms. It treats multidimensional arrays as vectors. Closes #12 * Try updating codecov action Added repository secret to GH repo config as well. * Disable test using feature not in v1.0 * Add tests, remove two nonsense methods Looks like I tried to add methods to satisfy JET or Aqua in the past. But they don't have arg combinations that could ever be called, even because of ambiguity. I removed them and all tests, including JET and Aqua still pass.
Merged
jlapeyre
added a commit
that referenced
this pull request
Oct 5, 2024
* Release new version with changes in PR #14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main change in the API is that functions in
IsApprox
with the same name as those in Base andLinearAlgebra
are now different methods of the same function. So you can dousing
of bothLinearAlgebra
andIsApprox
withouth qualifying any symbols.Another API change making the signatures uniform, rather than having the same argument in different places in different functions or methods. Making this change was enabled by the previous item.
A few details
Change argument order for Base.isapprox (Change argument order for Base.isapprox #13)
Previously, I had
Base.isapprox(::AbstractApprox, x, y)
. Now we useBase.isapprox(x, y, ::AbstractApprox)
. This order agrees with all other testing functions included in this package, such as,isone
,isreal
,ishermitian
, etc.All predicates either extend
Base
/LinearAlgebra
or are new to this packagePreviously, some predicates were copies of functions in
Base
/LinearAlgebra
, but did not extend these functions with new methods, but rather were new functions underIsApprox
. Other predicates inIsApprox
had no function inBase
/LinearAlgebra
of the same name.This commit changes this so that all predicate functions of the same name as those in
Base
/LinearAlgebra
now extend those functions.All predicate functions, including those extended from
LinearAlgebra
are exported.With this commit, you can do
using IsApprox
and suffer no name collisions withBase
/LinearAlgebra
.JET
andAqua
find no ambiguities or piracy.This commit and the previous, precursor commit, include a breaking change to the API.
Methods like
Base.isapprox(::AbstractApprox, x, y)
are nowBase.isapprox(x, y, ::AbstractApprox)
Methods likeBase.isapprox(x, y; approx::AbstractApprox)
are nowBase.isapprox(x, y, ::AbstractApprox)
In other words, the calling convention is now uniform.
Other changes
Add tests and fix small bug
Use 42.0 rather than 42 in isinteger test
A method for rationalize(::Int) does not exist in older versions (at leat 1.6) of Julia. So we use a float instead so that the test runs on more versions.
Fix bug in isunitary for Equal and IsApprox
Don't compare result to zero. Add one and compare to one. This is the proper scale because we expect the vectors to be normalized.
Explain new API
Fix some incorrect statements in comments
Base.isapprox
does not use matrix norms. It treats multidimensional arrays as vectors.Closes Calling convention #12
Try updating codecov action
Added repository secret to GH repo config as well.
Disable test using feature not in v1.0
Add tests, remove two nonsense methods
Looks like I tried to add methods to satisfy JET or Aqua in the past. But they don't have arg combinations that could ever be called, even because of ambiguity. I removed them and all tests, including JET and Aqua still pass.