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.

Sign up for GitHub

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

Jump to bottom

Normalize wavevector in thomson.spectral_density() #1190

Merged
merged 7 commits into from
Jul 11, 2021
Merged

Normalize wavevector in thomson.spectral_density() #1190

merged 7 commits into from
Jul 11, 2021

Conversation

bryancfoo
Copy link
Contributor

In calculation of the spectral density, wavevector k_vec was not unit normalized. Proper normalization noticeably changes the resultant spectra.

@bryancfoo bryancfoo requested a review from rocco8773 as a code owner July 9, 2021 19:06
@github-actions github-actions bot added the plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage label Jul 9, 2021
bryancfoo and others added 2 commits July 9, 2021 15:09
@bryancfoo
Create 1190.bugfix.rst
05a0c50
@pheuer
Fix broken test
3b131c3 
Function output is tested against a few fixed values: this one breaks with this fix.
@codecov
Copy link

codecov bot commented Jul 9, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@4dae984). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1190   +/-   ##
=======================================
  Coverage        ?   96.98%           
=======================================
  Files           ?       71           
  Lines           ?     7023           
  Branches        ?        0           
=======================================
  Hits            ?     6811           
  Misses          ?      212           
  Partials        ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dae984...2e56eb8. Read the comment docs.

@github-actions github-actions bot added the docs PlasmaPy Docs at http://docs.plasmapy.org label Jul 9, 2021
@pheuer pheuer changed the title Normalize wavevector Normalize wavevector in thomson.spectral_density() Jul 9, 2021
@pheuer
Copy link
Member

pheuer commented Jul 9, 2021

@rocco8773 This is a small bugfix, but since it does actually change the output of the function I'll wait to merge it until you take a look.

The one failing test seems to be unrelated: test_Chandrasekhar_with_hypothesis.

Thanks to @bryancfoo for spotting this error!

rocco8773
rocco8773 approved these changes Jul 11, 2021
View reviewed changes
Copy link
Member

@rocco8773 rocco8773 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryancfoo This looks good to me! Thank you! The failing checks are not related to any of the changes you made, so I'm going to merge the PR.

@StanczakDominik The predominate failing checks here are related to the hypothesis tests on Chandrasekhar G. I'm not sure of the cause because the tests passing/failing is sporadic. Thoughts?

@rocco8773 rocco8773 merged commit a1e96b0 into PlasmaPy:main Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rocco8773 rocco8773 rocco8773 approved these changes

Assignees
No one assigned
Labels
docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bryancfoo @pheuer @rocco8773