-
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
Bringing Modopt to 2024's standards. #340
Conversation
@paquiteau do you want me to review what you have done so far in this PR or wait till all the boxed are ticked? |
I would advise to start the review now, because the changelog is going to be very messy very fast. |
af304ae
to
13fcd25
Compare
random number generator stuff. This adds possibility to use rng properly in ModOpt.
Mutable class args. we don't want type annotations.
13fcd25
to
4956803
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #340 +/- ##
==========================================
- Coverage 90.58% 85.92% -4.67%
==========================================
Files 36 37 +1
Lines 2454 2010 -444
==========================================
- Hits 2223 1727 -496
- Misses 231 283 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cfbb6c9
to
1c87b73
Compare
@sfarrens The CI is now in a pretty good state, apart from the documentation setup. Could you have a look ? I am not mastering what you set up ^^ I think we should drop the MacOs test suite, it takes time, and does not bring that much of value to do the unit test in it. Maybe we still want to at least check if Modopt is installable in MacOs (with M1 chip notably). It remains out of my scope of competence though. |
@paquiteau amazing work (as always)! I will go through everything next week (I am at a meeting this week). For the macOS test suite we can simply make it manually triggered rather than automatically to reduce the per commit overhead. 🙂 |
@sfarrens any update on the review ? |
@paquiteau apologies for the delay. I will start going through it ASAP! |
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 only spotted a couple of very minor points. Once these threads are resolved I think we can merge. Nice work! 👏
Unless you have further changes planned of course. |
This PR goals is to:
src
layoutThis is going to be a big one...
Maybe the best is to merge previous PR (#338, #339) and fix the CI here. This supersed what @sfarrens started in #289.