-
Notifications
You must be signed in to change notification settings - Fork 335
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
Add enum
Module
#2713
Add enum
Module
#2713
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:
Important PlasmaPy recently switched to an 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! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2713 +/- ##
==========================================
+ Coverage 95.14% 95.18% +0.03%
==========================================
Files 107 107
Lines 9411 9487 +76
Branches 2168 2168
==========================================
+ Hits 8954 9030 +76
Misses 276 276
Partials 181 181 ☔ View full report in Codecov by Sentry. |
Will wait on #2710 to be merged so I can use the new decorator syntax to write the test (making sure all members of the enum are actually valid material strings) |
I definitely see why you'd do it this way, but I'm a little averse to creating a new module + enum just to support this one niche feature of the particle tracker. What if we just accepted a string for now, and said in the docstring that it had to be one of the entries in the h5 file? Or listed the acceptable strings at the end of the docstring? Or, you could put this enum in the proton tracker file? Just trying to manage complexity of the package. |
Ah this makes a lot more sense. I'll do that instead |
Actually, logically this should really be the same file as the What if you created |
Yeah that's probably a good idea especially given that I'm splitting the function into multiple routines in #2712. Should this be a public module? Or are you saying the |
I'd say public - we want the Bethe function to be public when you add that. And the Enum needs to be public if users are going to use it to specify materials |
@JaydenR2305 What's happening with this one? I think this enum should be in the same namespace as the function that gets the NIST data |
Speaking of which, where do you think these functions should be moved?? I don't think the atomic module is the best/most intuitive place |
The Bethe formula could maybe go in the formulary collisions module? Maybe all of it should go in a collisions/stopping.py file? Then we could add collisions/scattering.py later for those formulas? Definitely that would work for the Bethe formula - bit awkward to have the NIST function in the formulary, since it returns data (even though the data is output of analytical models, I think). But I don't know where else to put it? |
After discussion with @pheuer we are opting for a string input over an enumeration of all possible materials. A link to the PSTAR/ASTAR databases will be included in the relevant documentation to direct users to a list of acceptable material strings aside: the Bethe formula will be defined in/moved into |
Implements a new
enum
module to store enumerations for data types. Adds aStoppingMaterial
enum to be used with the NIST stopping power databaseshould probably wait until after #2712 is merged