Skip to content
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

Feature/update faer 0.21 #575

Merged
merged 11 commits into from
Mar 8, 2025
Merged

Conversation

geo-ant
Copy link
Contributor

@geo-ant geo-ant commented Mar 8, 2025

This adds support for faer-rs version 0.21.x, while keeping the previously added support for version 0.20. I realize, this looks like a daunting PR again in terms of lines changed, but it's actually not as bad. Since faer underwent a big rewrite between the versions, I had to factor out the actual implementations of the argmin-math traits into different modules and conditionally compile them. However, I also factored out the tests from the old faer_m module, which is now renamed to faer_m_0_20, to a common faer_test module. The tests were just factored out into this module, no changes were made to the tests*. That means the implementations for both faer backends pass the same, unaltered test suite.

In summary:

  • I renamed faer_m (and it's submodules to) faer_m_0_20
  • I factored out the tests in the submodules to faer_test
  • I added faer_m_0_21 for implementations in the 0.21 dependency

I hope newer faer versions will introduce less breaking changes for the fundamental operations we're using here, so that it won't be necessary to introduce faer_m_x_xx again. But even if not, this is reasonably simple to do.

(*) except at one place where I had to introduce a matrix_element_at helper function to access a matrix element across faer versions. But this is not a semantic change.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.10%. Comparing base (4728080) to head (9246d03).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #575      +/-   ##
==========================================
- Coverage   92.10%   92.10%   -0.01%     
==========================================
  Files         177      177              
  Lines       23675    23675              
==========================================
- Hits        21807    21806       -1     
- Misses       1868     1869       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@geo-ant
Copy link
Contributor Author

geo-ant commented Mar 8, 2025

urgh, the failing CI is related to #574 , which is about the paste crate being unmaintained as of recently.

@stefan-k
Copy link
Member

stefan-k commented Mar 8, 2025

Thanks for this PR, it's great to have the most recent version supported as well! :)

urgh, the failing CI is related to #574 , which is about the paste crate being unmaintained as of recently.

"urgh" is the perfect noise for this. About 90% of the time I have available for argmin is spent on things like this ;) It should be fixed in main after merging #576

@geo-ant
Copy link
Contributor Author

geo-ant commented Mar 8, 2025

Thanks for this PR, it's great to have the most recent version supported as well! :)

urgh, the failing CI is related to #574 , which is about the paste crate being unmaintained as of recently.

"urgh" is the perfect noise for this. About 90% of the time I have available for argmin is spent on things like this ;) It should be fixed in main after merging #576

I feel you, and I hope you didn't take it as an insult to you. You have your CI set up really professionally, I'm going to take a page of your playbook for my projects at some point.

@stefan-k
Copy link
Member

stefan-k commented Mar 8, 2025

I feel you, and I hope you didn't take it as an insult to you.

No no, not at all. I just felt like venting a little :D

You have your CI set up really professionally, I'm going to take a page of your playbook for my projects at some point.

Thanks and feel free to do so. Don't be surprised if you find a mess in certain corners ;)

@stefan-k stefan-k merged commit d4f20b7 into argmin-rs:main Mar 8, 2025
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants