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.

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

Closed
wants to merge 11 commits into from
Closed

Conversation

JaydenR2305
Copy link
Member

@JaydenR2305 JaydenR2305 commented Jun 3, 2024

Implements a new enum module to store enumerations for data types. Adds a StoppingMaterial enum to be used with the NIST stopping power database

should probably wait until after #2712 is merged

Copy link

github-actions bot commented Jun 3, 2024

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 src layout. The source code previously in plasmapy/ is now in src/plasmapy/. Tests are now in tests/. If you have previously done an editable installation, it will likely need to be re-done (i.e., with pip install -e .[tests,docs] in the top-level directory of the repository). The former plasmapy/ directory will need to be deleted manually in old clones because git does not track directories.

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:

  • Try fixing CI / Python 3.12 test failures first.
  • Most pre-commit.ci - pr failures can be automagically fixed by commenting pre-commit.ci autofix below, followed by a git pull to bring the changes back to your computer. Please also see our pre-commit troubleshooting guide.
  • If pre-commit.ci - pr says that a function is too long or complex, try breaking up that function into multiple short functions that each do one thing. See also these tips on writing clean scientific software.
  • If the CI / Documentation check ends with a cryptic error message, check out our documentation troubleshooting guide.
  • For a documentation preview, click on Details next to docs/readthedocs.org:plasmapy.

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!

@github-actions github-actions bot added docs PlasmaPy Docs at https://docs.plasmapy.org python Pull requests that update Python code labels Jun 3, 2024
@github-actions github-actions bot added the feature For new functionality, excluding breaking changes label Jun 3, 2024
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.18%. Comparing base (71663fb) to head (995a7d4).
Report is 7 commits behind head on main.

Current head 995a7d4 differs from pull request most recent head 21d9382

Please upload reports for the commit 21d9382 to get more accurate results.

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.
📢 Have feedback on the report? Share it here.

@JaydenR2305
Copy link
Member Author

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)

@pheuer
Copy link
Member

pheuer commented Jun 3, 2024

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.

@JaydenR2305
Copy link
Member Author

Or, you could put this enum in the proton tracker file?

Ah this makes a lot more sense. I'll do that instead

@github-actions github-actions bot added testing plasmapy.simulation Related to the plasmapy.simulation subpackage labels Jun 3, 2024
@pheuer
Copy link
Member

pheuer commented Jun 5, 2024

Or, you could put this enum in the proton tracker file?

Ah this makes a lot more sense. I'll do that instead

Actually, logically this should really be the same file as the stopping_power function, so that they appear in the docs together? The problem is, that's already in quite a large file particles/atomic...

What if you created _stopping_power.py in particles and moved both the stopping_power function and this associated enum there? @namurphy what do you think about the best way to organize this?

@JaydenR2305
Copy link
Member Author

What if you created _stopping_power.py in particles and moved both the stopping_power function and this associated enum there? @namurphy what do you think about the best way to organize this?

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 enum should only be accessible by the particles subpackage (plasmapy.particles.StoppingMaterial)

@pheuer
Copy link
Member

pheuer commented Jun 5, 2024

What if you created _stopping_power.py in particles and moved both the stopping_power function and this associated enum there? @namurphy what do you think about the best way to organize this?

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 enum should only be accessible by the particles subpackage (plasmapy.particles.StoppingMaterial)

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

@pheuer
Copy link
Member

pheuer commented Jun 14, 2024

@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

@JaydenR2305
Copy link
Member Author

@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

@github-actions github-actions bot added the plasmapy.particles Related to the plasmapy.particles subpackage label Jun 21, 2024
@pheuer
Copy link
Member

pheuer commented Jun 21, 2024

@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?

@JaydenR2305
Copy link
Member Author

JaydenR2305 commented Jun 27, 2024

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 formulary.collisions.misc

@JaydenR2305 JaydenR2305 deleted the materials-enumeration branch June 27, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PlasmaPy Docs at https://docs.plasmapy.org feature For new functionality, excluding breaking changes plasmapy.particles Related to the plasmapy.particles subpackage plasmapy.simulation Related to the plasmapy.simulation subpackage python Pull requests that update Python code testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants