-
Notifications
You must be signed in to change notification settings - Fork 307
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
Modify the way settings are passed to the _spectral_density_model function #2623
Modify the way settings are passed to the _spectral_density_model function #2623
Conversation
Thank you for submitting a pull request (PR) to PlasmaPy! ✨ The future of the project depends on contributors like you, so we deeply appreciate it! 🌱 Our contributor guide has information on:
The bottom of this page shows several checks that are run for every PR. Don't worry if something broke! We break stuff all the time. 😺 Click on "Details" to learn why a check didn't pass. Please also feel free to ask for help. We do that all the time as well. 🌸 You can find us in our chat room or weekly community meeting & office hours. Here are some tips:
If this PR is marked as ready for review, someone should stop by to provide a code review and offer suggestions soon. ✅ If you don't get a review within a few days, please feel free to send us a reminder. Please also use SI units within PlasmaPy, except when there is strong justification otherwise or in some examples. We thank you once again! |
@namurphy This should be good to go, so merge when ready! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2623 +/- ##
=======================================
Coverage 95.17% 95.17%
=======================================
Files 104 104
Lines 9415 9417 +2
Branches 2154 2154
=======================================
+ Hits 8961 8963 +2
Misses 276 276
Partials 178 178 ☔ 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.
LGTM! 👍🏻 Thank you again for doing this! 🚀
One small optional suggestion, and feel free to merge whenever you're ready!
@@ -1000,10 +1000,12 @@ def spectral_density_model( # noqa: C901, PLR0912, PLR0915 | |||
# quantities isn't consistent with the number of that species defined | |||
# by ifract or efract. | |||
|
|||
def _spectral_density_model_lambda(wavelengths, **params): |
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.
def _spectral_density_model_lambda(wavelengths, **params): | |
def _spectral_density_model_wrapper(wavelengths, **params): |
❔ 🏷️ Since this isn't defined with a lambda
statement, what would you think of renaming it as a wrapper?
Also I'm trying out prefixing code review comments with emojis! I'm still figuring it out, but I'm using ❔ to indicate that it's optional and up to you, and 🏷️ refers to naming.
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.
Oops, too late, looks like it's already merged but sure I'm fine with either name! I originally had it as a lambda, but switched to this syntax just to keep it under the line limit
# Create and return the lmfit.Model | ||
return Model( | ||
_spectral_density_model, | ||
_spectral_density_model_lambda, |
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.
_spectral_density_model_lambda, | |
_spectral_density_model_wrapper, |
@namurphy codecov token issue?
|
NOOoooOOOoooOOOOooOOOOO!!!!!!! 😿😿😿😿😿😿😿😿 I thought I had fixed that! Let's...ignore codecov checks until we get them working again. Thank you for pointing this out! |
Also I realized that my response should have technically been "YEEEeeeEEEeeeeSSSSsssSSSS!" 👀 😹 |
…gs_thomson_workaround
Addresses #2620 without capping the version of
lmfit
Supercedes #2621
Instead of passing
settings
toModel
and relying onlmfit
to pass that to_spectral_density_model
, this PR uses a lambda function to freeze in the settings (which are constant) prior to giving the function tolmfit
.