-
Notifications
You must be signed in to change notification settings - Fork 101
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
Brute force method to find the optimal number of harmonics for WaveX
, DMWaveX
, and CMWaveX
#1824
base: master
Are you sure you want to change the base?
Conversation
WaveX
, DMWaveX
, and CMWaveX
WaveX
, DMWaveX
, and CMWaveX
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1824 +/- ##
==========================================
+ Coverage 69.53% 69.70% +0.16%
==========================================
Files 108 110 +2
Lines 25139 25407 +268
Branches 4458 4520 +62
==========================================
+ Hits 17480 17709 +229
- Misses 6547 6565 +18
- Partials 1112 1133 +21 ☔ View full report in Codecov by Sentry. |
@@ -10,3 +10,4 @@ uncertainties | |||
loguru | |||
nestle>=0.2.0 |
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.
should we discuss having a new requirement more broadly? If this is only needed for a subset of tasks, should it be optional?
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 personally fine with adding new requirements if the requirement is pure python and doesn't have a bunch of its own new requirements. nestle
seems like it is of that (good) type.
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 think this was actually adding joblib
?
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.
In particular, I wonder about how using joblib
compares to the use of concurrent.futures
? I see some discussion online. It seems like we might want to stick with one library for that functionality.
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 will try concurrent.futures
.
This method will be slow since this is brute force. I have some ideas for a faster version, but that will be in another PR.
This PR depends on #1802, so it should be merged before merging this.