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

Replacing PyNE with radioactivedecay for radionuclide decay calculations #1813

Merged
merged 12 commits into from Apr 11, 2022

Conversation

lemointm
Copy link
Contributor

This PR is designed to replace the functionality of PyNE in TARDIS with the python package radioactivedecay.

Description

TARDIS uses PyNE to perform decay caluclations, and to handle nuclide name parsing. The main change will be with handling nuclide decays in tardis/io/decay.py with radioactivedecay, and to replace the usage of PyNE's nucname function with the radioactivedecay.Nuclide class' name parsing functionality throughout TARDIS. Nuclide decays will now use the radioactivedecay.Inventory objects and the decay() method.

Motivation and context

This PR is meant to solve problems outlined in issue #1654.

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

1 similar comment
@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

tardis_env3.yml Outdated Show resolved Hide resolved
tardis_env3.yml Outdated Show resolved Hide resolved
tardis/io/decay.py Outdated Show resolved Hide resolved
@andrewfullard andrewfullard marked this pull request as ready for review February 17, 2022 22:57
@andrewfullard
Copy link
Contributor

Need to format to black standards

@wkerzendorf
Copy link
Member

@lemointm the tests are failing - can you make sure it reruns.

@lemointm
Copy link
Contributor Author

Just pushed a commit for formatting, I will check the other tests next

@afloers
Copy link
Contributor

afloers commented Feb 28, 2022

pyne also needs to be replaced in the tests

ImportError while loading conftest '/home/vsts/work/1/s/tardis/tardis/conftest.py'.
tardis/init.py:15: in
import pyne.data
E ModuleNotFoundError: No module named 'pyne'

@epassaro
Copy link
Member

There are several references to pyne in the tardis package, use Ctrl+Shift+F on VSCode to find them.

tardis/util/base.py Outdated Show resolved Hide resolved
@lemointm
Copy link
Contributor Author

lemointm commented Mar 2, 2022

There's an ongoing test failure where radioactivedecay says Ni-58 doesn't exist, this is a small issue with the package due to Ni-58 not being a progeny of any of the nuclides in the dataset (since it's produced by spontaneous fission only). I will try to get this fixed in radioactivedecay asap.

tardis/io/decay.py Outdated Show resolved Hide resolved
tardis/model/base.py Outdated Show resolved Hide resolved
tardis/visualization/widgets/custom_abundance.py Outdated Show resolved Hide resolved
tardis/visualization/widgets/custom_abundance.py Outdated Show resolved Hide resolved
@lemointm
Copy link
Contributor Author

@andrewfullard I added a util to tardis to perform those checks that I had rewritten in a few different places. The code should be a little cleaner now.

@lemointm
Copy link
Contributor Author

@andrewfullard I added a util to tardis to perform those checks that I had rewritten in a few different places. The code should be a little cleaner now.

I spoke too soon, I just discovered that radioactivedecay can input grams, commit coming shortly!

@lemointm
Copy link
Contributor Author

There also was an error where Inventory.mass_fractions() wasn't working properly since each nuclide is stored in a different Inventory. This was fixed by switching to Inventory.masses('g') instead, with an update to having inventories stored in grams.

@lemointm
Copy link
Contributor Author

Now there should be only the Ni-58 test error (for now). I will give an update once the fixes are made to radioactivedecay.

@lemointm
Copy link
Contributor Author

After the newest release (0.4.12) to radioactivedecay, all tests pass. I marked the code for review, please let me know if there are any other changes anyone would like to see.

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #1813 (c99a4a0) into master (f541e73) will increase coverage by 0.05%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master    #1813      +/-   ##
==========================================
+ Coverage   58.74%   58.80%   +0.05%     
==========================================
  Files          70       70              
  Lines        7847     7853       +6     
==========================================
+ Hits         4610     4618       +8     
+ Misses       3237     3235       -2     
Impacted Files Coverage Δ
tardis/tardis/io/logger/logger.py 70.68% <0.00%> (-1.45%) ⬇️
tardis/tardis/io/atom_data/base.py 89.71% <0.00%> (-0.06%) ⬇️
tardis/tardis/model/base.py 88.92% <0.00%> (-0.04%) ⬇️
tardis/tardis/io/decay.py 89.65% <0.00%> (ø)
tardis/tardis/plasma/properties/atomic.py 53.12% <0.00%> (ø)
...rdis/plasma/properties/transition_probabilities.py 24.03% <0.00%> (ø)
tardis/tardis/io/model_reader.py 97.77% <0.00%> (+0.06%) ⬆️
...s/tardis/visualization/widgets/custom_abundance.py 14.14% <0.00%> (+0.11%) ⬆️
...s/tardis/montecarlo/montecarlo_numba/estimators.py 34.61% <0.00%> (+0.13%) ⬆️
...dis/montecarlo/montecarlo_numba/formal_integral.py 51.36% <0.00%> (+0.16%) ⬆️
... and 2 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Rodot-
Copy link
Contributor

Rodot- commented Mar 28, 2022

Can you update the astropy version to >=4.1 to remove the distutils warnings?

@lemointm
Copy link
Contributor Author

Can you update the astropy version to >=4.1 to remove the distutils warnings?

I also added radioactivedecay>=0.4.12 to ensure that the latest version with Ni-58 is installed.

@lemointm
Copy link
Contributor Author

There still seems to be an issue with astroPy, now with importing astropy.modeling.blackbody. The documentation mentions that the module are deprecated as of version 4.0.

@lemointm lemointm mentioned this pull request Mar 30, 2022
10 tasks
@andrewfullard
Copy link
Contributor

Reminder to revert the astropy version change in this PR so we can merge it

@epassaro
Copy link
Member

epassaro commented Apr 4, 2022

Also, please try to build the documentation before merging!

@epassaro
Copy link
Member

epassaro commented Apr 4, 2022

Black check is failing.

@andrewfullard
Copy link
Contributor

Black check is failing.

#1940 is the reason

@epassaro
Copy link
Member

epassaro commented Apr 6, 2022

Black check is failing.

#1940 is the reason

Thanks, I think we should update black in https://github.com/tardis-sn/tardis/blob/master/.github/workflows/black-check.yml instead of doing in the env file.

@lemointm
Copy link
Contributor Author

lemointm commented Apr 6, 2022

I just reverted the astropy version, and ran the docs on my machine - everything looks good!

@@ -14,7 +14,7 @@ dependencies:
- astropy=3
- numba=0.53
- numexpr
- pyne=0.7=nomoab_openmc* # Issue #1537
- radioactivedecay>=0.4.12
Copy link
Member

@epassaro epassaro Apr 11, 2022

Choose a reason for hiding this comment

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

Is a good idea to pin the bugfix number? Maybe just minor version it's ok, what do you think @lemointm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to Alex about this, we think that in the future the functionality TARDIS needs won't get deprecated

Copy link
Member

Choose a reason for hiding this comment

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

but that shouldn't affect the bugfix number, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you're saying, sorry I misunderstood. The "bugfix" added some functionality, so it needs to be pinned... which is not the standard but that's how it ended up I suppose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants