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

Return NaN values instead of throw exceptions for particle attributes #1825

Merged
merged 5 commits into from Dec 11, 2022

Conversation

Sobeskes
Copy link
Contributor

@Sobeskes Sobeskes commented Dec 8, 2022

Description

In particle_class, NaN values are now returned instead of throwing exceptions.

In addition, test_atomic, test_exceptions, and test_particle_class are changed to reflect changes to particle_class and new tests are added to test_particle_class to ensure that NaN values are returned when accessing undefined properties.

Motivation and context

By returning NaN values, numpy.nan can keep track of what is undefined and what is not, avoiding dealing with exceptions as a special case. Also makes the behavior of Particles and CustomParticles more consistent

Related issues

This issue was brought up in #1016

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

Thank you for contributing to PlasmaPy! The project's future depends deeply on contributors like you, so we deeply appreciate it! 🌱 The following checklist will be used by the code reviewer to help guide the code review process.

  • Overall
    • Does the PR do what it intends to do?
    • Except for very minor changes, is a changelog entry included and consistent with the changelog guide?
    • Are the continuous integration checks passing? (Most linter problems can be automagically fixed by commenting on this PR with pre-commit.ci autofix.)
  • Code
    • Is new/updated code understandable and consistent with the coding guide?
    • Are there ways to greatly simplify the implementation?
    • Are there any large functions that should be split up into shorter functions?
  • Tests
    • Are tests added/updated as required, and consistent with the testing guide?
    • Are the tests understandable?
    • Do the tests cover all important cases?
  • Docs
    • Are docs added/updated as required, and consistent with the doc guide?
    • Are the docs understandable?
    • Do the docs show up correctly in the preview, including Jupyter notebooks?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for making your first contribution to PlasmaPy! The project's future depends on contributors like you, so we deeply appreciate it! 🌱

To make sure that you get credit for this pull request (PR), please add yourself to the list of contributors in docs/about/credits.rst.

We encourage you to check out our contributor guide, which discusses topics like adding changelog entries. If you'd like to talk with us in real time, the easiest way is via our chat room. We also invite you to stop by our weekly community meeting or office hours.

The bottom of this page includes several checks that are run for every PR. Don't worry if something broke! We break stuff all the time. Eventually we'll want to get checks on this PR to pass before merging it. Clicking on Details next to the check will show the results of the test, including error messages. Please feel free to ask for help if you're having trouble (we do that all the time, as well).

Our testing guide describes these checks in more detail, as well as how to write and run tests. We recommend trying out test-driven development: write tests first, and then write code to make tests pass. Remember to run tests often!

As described in our documentation guide, PlasmaPy's documentation is written in reStructuredText using the numpydoc docstring standard. To see a preview of the documentation from this PR, click on Details next to the docs/readthedocs.org:plasmapy check.

To automagically fix most linter problems, comment on this PR with pre-commit.ci autofix, followed by a git pull to bring the changes home to your own computer.

A code maintainer should come by soon to begin a code review. This step often includes suggestions for fixing bugs, adding tests, or simplifying the implementation. After that you'll have a chance to make changes. If you'd like more time before a code review happens, you can convert this PR to a draft now and mark it as ready for review later.

Finally, we try to be the best community we can be. We have a Code of Conduct to foster a community of care and appreciation.

We thank you once again! ✨

@namurphy
Copy link
Member

namurphy commented Dec 8, 2022

Thank you for submitting this! The Python 3.10 tests are failing because of an unrelated issue, which should be resolved once #1822 is merged.

Copy link
Member

@namurphy namurphy left a comment

Choose a reason for hiding this comment

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

Looks good to me! I have only a few minor formatting suggestions, and the only other changes are to add a changelog entry and then add your name to the list of authors in docs/about/credits.rst if you haven't done so already. After that, it should be ready to go as soon as we make sure the tests pass. That might mean doing a git pull upstream main (where upstream is the git remote for PlasmaPy's repository) and then a git push. Thank you for doing this!

plasmapy/particles/particle_class.py Outdated Show resolved Hide resolved
plasmapy/particles/particle_class.py Outdated Show resolved Hide resolved
plasmapy/particles/tests/test_particle_class.py Outdated Show resolved Hide resolved
Copy link
Member

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

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

PR looks great and so do Nick's suggestions :) I'm happy to see it go in once those are done.

Sobeskes and others added 2 commits December 10, 2022 15:17
Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>
@Sobeskes Sobeskes requested a review from a team as a code owner December 10, 2022 22:06
changelog/1825.feature.rst Outdated Show resolved Hide resolved
@StanczakDominik
Copy link
Member

pre-commit.ci autofix

@StanczakDominik StanczakDominik enabled auto-merge (squash) December 11, 2022 19:17
@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Base: 98.35% // Head: 98.10% // Decreases project coverage by -0.24% ⚠️

Coverage data is based on head (ded0419) compared to base (f10eede).
Patch coverage: 50.00% of modified lines in pull request are covered.

❗ Current head ded0419 differs from pull request most recent head ef6864e. Consider uploading reports for the commit ef6864e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1825      +/-   ##
==========================================
- Coverage   98.35%   98.10%   -0.25%     
==========================================
  Files          91       94       +3     
  Lines        8246     8304      +58     
==========================================
+ Hits         8110     8147      +37     
- Misses        136      157      +21     
Impacted Files Coverage Δ
plasmapy/particles/particle_class.py 98.49% <50.00%> (-0.51%) ⬇️
plasmapy/formulary/lengths.py 100.00% <0.00%> (ø)
plasmapy/analysis/time_series/running_moments.py 100.00% <0.00%> (ø)
plasmapy/__init__.py 33.33% <0.00%> (ø)
plasmapy/analysis/time_series/__init__.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@StanczakDominik StanczakDominik merged commit c625b6e into PlasmaPy:main Dec 11, 2022
@namurphy namurphy added the plasmapy.particles Related to the plasmapy.particles subpackage label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plasmapy.particles Related to the plasmapy.particles subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants