-
Notifications
You must be signed in to change notification settings - Fork 305
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
Implement NIST STAR Data #2555
Implement NIST STAR Data #2555
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 I wasn't quite sure where to put this function.. Where do you think it should go? |
Add citation to NIST STAR
…o add_nist_star_functionality
…st_star_functionality # Conflicts: # plasmapy/utils/data/downloader.py
@pheuer Do you know what's causing these ReadTimeout errors?? |
Hm, does the test pass locally? The file/url it is trying to reach is definitely valid? One thing you might want to do to limit test time/number of API queries is to use an unvalidated downloader in the tests (except for maybe one, that confirms the file is accessible). See example of how I did that here:
But, that shouldn't explain the read timeout, since even in the unvalidated mode |
Does this mean we would take a |
Or you could add a switch inside It might be best practice to just say that any function that internally uses Downloader should allow an instance to be passed as an optional argument, just for this kind of thing, though? |
@pheuer What are your thoughts on implicitly setting the API token if it is detected that the code is being run in a CI environment? That is, if an API key is not explicitly passed during instantiation then This way we don't need to complicate things by introducing optional keywords for |
Hm, that's a good idea for now! It's not exactly equivalent to unvalidated (still makes queries to find the file I think) but that's ok. Make sure you include a comment in the test explaining what you're doing. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
==========================================
- Coverage 95.25% 95.21% -0.04%
==========================================
Files 104 104
Lines 9435 9435
Branches 2159 2167 +8
==========================================
- Hits 8987 8984 -3
- Misses 272 273 +1
- Partials 176 178 +2 ☔ View full report in Codecov by Sentry. |
How should I add coverage for these lines? PlasmaPy/src/plasmapy/particles/atomic.py Lines 1238 to 1241 in fe4ed92
The returned result can have some ~100 entries. |
also why does this have partial coverage? It looks like the code under the elseif statement is being executed fine? https://app.codecov.io/gh/PlasmaPy/PlasmaPy/pull/2555/blob/src/plasmapy/particles/atomic.py#L595 |
It looks like this isn't being hit just because all the test cases supply
I've no idea about this, weird... |
material_data["electronic_stopping_power"] | ||
+ material_data["nuclear_stopping_power"] | ||
) | ||
elif component == "electronic": |
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.
These two branches aren't getting hit: I would add one more test/test case that asks for the electronic and then the nuclear stopping power
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
Looks good!
Implements the ability to calculate stopping power for alpha particles and protons in a specified material using the NIST ASTAR and PSTAR databases, respectively. The data for this pull request can be found here
Resolves #2517