Adding Bdot probe diagnostic and corresponding test#3055
Adding Bdot probe diagnostic and corresponding test#3055mirfan4906 wants to merge 18 commits intoPlasmaPy:mainfrom
Conversation
|
Thank you for submitting this pull request (PR)! ✨ Below are checks that are being run for this PR. Click on the name of a check to learn why it didn't pass. ✅ Don't worry if something broke! We break stuff all the time. 😅 PlasmaPy's contributor guide has pages on:
Tip 📚 For a documentation preview, click on docs/readthedocs.org:plasmapy below. For cryptic documentation errors, see the documentation troubleshooting guide. Tip 🧹 Automatically fix most pre-commit.ci failures by commenting We thank you once again! 🌌 |
github-actions
bot
added
testing
plasmapy.diagnostics
Codecov Report❌ Patch coverage is Please upload reports for the commit 08f68ee to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #3055 +/- ##
==========================================
+ Coverage 94.71% 95.46% +0.75%
==========================================
Files 108 109 +1
Lines 9869 9887 +18
Branches 1501 1502 +1
==========================================
+ Hits 9347 9439 +92
+ Misses 325 256 -69
+ Partials 197 192 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
namurphy
left a comment
namurphy
left a comment
There was a problem hiding this comment.
Thank you so much for submitting this contribution! Given how widely used bdot probes are in experiments and as space plasma diagnostics, this is going to be very beneficial for the community.
- I provided some thoughts and suggestions below, including several minor changes.
- Could you add yourself as an author to
CITATION.cff? That will help make sure you get credit for the contribution. - Could you add a changelog entry at
changelog/3055.feature.rst? - There are a few tedious things that we need to do when adding a new
.pyfile, which can wait until later. In #3023, I started to add a section to the contributor guide that includes a checklist of what needs to be done, but I still need to finish it. 😅 Maybe this week?
Thank you once again! We really appreciate it!
| # Use a tolerance since numerical integration introduces small errors | ||
| assert u.allclose(computed_bfield, expected_field, rtol=1e-3, atol=1e-5 * u.T) |
There was a problem hiding this comment.
It was a good idea here to explain the relatively high value for rtol.
| if len(bdot_voltage) != len(time_array): | ||
| raise Exception("length of time and voltage arrays in not equal\n") |
There was a problem hiding this comment.
Good idea to check this!
The Codecov check is showing that the code in this block is not being run during any tests. Could we add another test to make sure that this exception gets raised as appropriate? It's discussed in the testing guide, and would be along the lines of:
def test_compute_bfield_error():
time_array = ...
bdot_voltage = ... # has a different number of elements than time
with pytest.raises(ValueError):
compute_bfield(time_array, bdot_voltage, ...)Thank you!
There was a problem hiding this comment.
I’ll review the testing guide and implement the additional test you suggested. Thank you so much for the thorough and helpful feedback. It has made me much more familiar with the format and process, which hopefully will make things smoother as I move forward with Professor Schaffner. I’ve made the other code changes and will be going through the remaining instructions as well. I'll be on the lookout for any other instructions/tasks regarding submitting a new pull request!
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Nick Murphy <[email protected]>
2 participants
Implementation Details
compute_bfieldis located insrc/plasmapy/diagnostics/magnetic_pickup_probe.py.Tests
tests/diagnostics/test_magnetic_pickup_probe.py.