-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add nalgebra 0.33 support #505
Conversation
Hi, thanks also for this PR! Would you mind rebasing now that #504 is merged? |
d1f9bdf
to
0807366
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
==========================================
- Coverage 92.08% 91.49% -0.60%
==========================================
Files 178 178
Lines 24455 23623 -832
==========================================
- Hits 22520 21613 -907
- Misses 1935 2010 +75 ☔ View full report in Codecov by Sentry. |
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 a lot for this work, this is highly valuable! :) I have one question out of curiosity, otherwise this is good to go!
crates/argmin-math/Cargo.toml
Outdated
# simba | ||
simba_0_6 = { package = "simba", version = "0.6", optional = true } | ||
simba_0_7 = { package = "simba", version = "0.7", optional = true } | ||
simba_0_8 = { package = "simba", version = "0.8", optional = true } | ||
simba_0_9 = { package = "simba", version = "0.9", optional = true } |
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'm not an nalgebra expert, could you explain to me why simba is needed as a dependency?
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.
simba is already a dependency of nalgebra, it's just that nalgebra 0.33 no longer re-exports certain traits used in this crate like simba::scalar::ClosedAdd
.
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.
Actually taking a second look, I'm just replacing ClosedAdd
with ClosedAddAssign
which is reexported in 0.33, so I could remove simba and use the trait from nalgebra instead.
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 its not necessary I'd prefer it if it were removed, simply to avoid having to manage it.
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.
Ok, I've removed it.
Could you add testing the 0.33 version to the CI, please? argmin/.github/workflows/ci.yml Lines 120 to 130 in be2444e
|
0807366
to
168d918
Compare
any update? |
Hi @stefan-k, it appears that @0e4ef622 added CI support in 0e4ef622@168d918. |
Apologies for being so absent recently. I'll try to address some of the open pull requests on Sunday. |
No problem at all, we all get busy! 😄 Thank you for everything you do! |
Thanks for the kind words! :) |
Since the
Allocator
trait changed, I added a trait alias to mimic the oldAllocator
trait.