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

Hollweg values test using arrays for k and theta #1549

Merged
merged 33 commits into from
May 26, 2022
Merged

Hollweg values test using arrays for k and theta #1549

merged 33 commits into from
May 26, 2022

Conversation

Sjbrownian
Copy link
Contributor

@Sjbrownian Sjbrownian commented May 9, 2022 •
edited by rocco8773
Loading

  • I have added a changelog entry for this pull request.
  • If adding new functionality, I have added tests and docstrings.
  • I have fixed any newly failing tests.

Closes #1542

Sjbrownian and others added 24 commits April 25, 2022 13:42
@Sjbrownian
Fix bug
1b71d04
@Sjbrownian
Merge branch 'PlasmaPy:main' into hollweg
8fad23e
@Sjbrownian
Create 1529.bugfix.rst
3b24259
@Sjbrownian
Update 1529.bugfix.rst
0e03c3f
@Sjbrownian
warning
6514860
@Sjbrownian
Merge branch 'hollweg' of https://github.com/Sjbrownian/PlasmaPy into…
b0e36d9 
… hollweg
@Sjbrownian
added test for multiple k/theta vals
59e8ece
@Sjbrownian
test
438fc78
@Sjbrownian
theta warning change
586a473
@Sjbrownian
single k multiple theta case
f9d1935
@Sjbrownian @StanczakDominik
Update plasmapy/dispersion/numerical/hollweg_.py
6608ea6 
Co-authored-by: Dominik Stańczak <[email protected]>
@Sjbrownian @rocco8773
Update plasmapy/dispersion/numerical/hollweg_.py
c9a9a17 
Co-authored-by: Erik Everson <[email protected]>
@Sjbrownian @rocco8773
Update changelog/1529.bugfix.rst
480f9a7 
Co-authored-by: Erik Everson <[email protected]>
@Sjbrownian
test_vals added
0578ebf
@Sjbrownian
Merge branch 'hollweg' of https://github.com/Sjbrownian/PlasmaPy into…
0c60694 
… hollweg
@Sjbrownian
Merge branch 'PlasmaPy:main' into hollweg
7f60b94
@Sjbrownian
Create 1549.trivial.rst
c0e6d8a
@Sjbrownian
bug fix
41ad898
@Sjbrownian
Merge branch 'hollweg' of https://github.com/Sjbrownian/PlasmaPy into…
9e43e87 
… hollweg
@Sjbrownian
isclose
d5fe916
@Sjbrownian
allclose
cbd0df3
@Sjbrownian
remove extra coma
07ca91c
@Sjbrownian
remove assert
9fc3b06
@Sjbrownian
new test case added
188448e
@codecov
Copy link

codecov bot commented May 9, 2022 •
edited
Loading

Codecov Report

Merging #1549 (14b623f) into main (2a13801) will increase coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 14b623f differs from pull request most recent head 7f6aa15. Consider uploading reports for the commit 7f6aa15 to get more accurate results

@@            Coverage Diff             @@
##             main    #1549      +/-   ##
==========================================
+ Coverage   97.09%   97.19%   +0.09%     
==========================================
  Files          83       82       -1     
  Lines        7676     7902     +226     
==========================================
+ Hits         7453     7680     +227     
+ Misses        223      222       -1
Impacted Files Coverage Δ
plasmapy/diagnostics/thomson.py 97.95% <0.00%> (-2.05%) ⬇️
plasmapy/particles/_parsing.py 98.44% <0.00%> (-1.56%) ⬇️
plasmapy/particles/__init__.py 95.83% <0.00%> (-0.33%) ⬇️
plasmapy/plasma/grids.py 98.92% <0.00%> (ø)
plasmapy/utils/__init__.py 100.00% <0.00%> (ø)
plasmapy/formulary/__init__.py 100.00% <0.00%> (ø)
plasmapy/formulary/collisions.py 96.96% <0.00%> (ø)
plasmapy/formulary/parameters.py 100.00% <0.00%> (ø)
plasmapy/formulary/frequencies.py 100.00% <0.00%> (ø)
plasmapy/dispersion/numerical/hollweg_.py 100.00% <0.00%> (ø)
... and 6 more

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 2a13801...7f6aa15. Read the comment docs.

@Sjbrownian
Copy link
Contributor Author

@rocco8773 Hi Erik, I've added some tests (test_vals) for dealing with array arguments for k and theta in hollweg but I'm getting an error when using assert. I've spent some time messing around with it to see if I could figure out why the assert was failing but I'm unsure what's causing it. Would you be able to take a look sometime?

I have finals to focus on for the next week or two so I may not be at the next meeting. If you feel the issue isn't an easy fix and we should discuss in person in two weeks or so I'm fine with that as well.

rocco8773
rocco8773 requested changes May 11, 2022
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.

This looks to be your culprit. The default units for theta in hollweg() are radians, but you're specifying a value in degrees without the units attached. Thus, hollweg is using 88 radians instead of 88 degrees.

plasmapy/dispersion/numerical/tests/test_hollweg_.py Outdated Show resolved Hide resolved
plasmapy/dispersion/numerical/tests/test_hollweg_.py Outdated Show resolved Hide resolved
@Sjbrownian Sjbrownian mentioned this pull request May 26, 2022
Merged
3 tasks
rocco8773
rocco8773 reviewed May 26, 2022
View reviewed changes
plasmapy/dispersion/numerical/tests/test_hollweg_.py
( # array input for k, single value for theta
{
**_kwargs_single_valued,
"k": np.logspace(-7, -2, 2) * u.rad / u.m,
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using negative values for k here? Since we're using k and theta as input arguments, I would think we would require k > 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's np.logspace so it should range from [10^-7,10^-2] and be positive right?

Copy link
Member

Choose a reason for hiding this comment

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

ah...I see...my brain automatically read np.linspace

rocco8773
rocco8773 previously approved these changes May 26, 2022
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.

This looks good! Thank you for doing this.

I just made some minor tweaks to make a few things more explicit. Once I confirm the docs build correctly, I'll go ahead and merge the PR.

@rocco8773 rocco8773 enabled auto-merge (squash) May 26, 2022 18:59
@rocco8773 rocco8773 merged commit 1b92384 into PlasmaPy:main May 26, 2022
@Sjbrownian Sjbrownian deleted the hollweg branch May 26, 2022 19:09
@namurphy namurphy added plasmapy.dispersion Related to the plasmapy.dispersion subpackage testing labels May 23, 2023
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

@rocco8773 rocco8773

Labels
plasmapy.dispersion Related to the plasmapy.dispersion subpackage testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create value test for hollweg using arrays for k and theta.
3 participants
@Sjbrownian @rocco8773 @namurphy