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

Improved point and ring sources #51

Conversation

LiamPattinson
Copy link
Contributor

I've updated FusionPointSource and FusionRingSource to store their inputs as properties, which allows them to be checked for validity. Info on DT/DD fuel types has been moved to a new class, which may be refactored later if work on Issue #37 goes ahead. I've also added unit tests for these new features.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #51 (4c80c3d) into develop (e430db5) will increase coverage by 2.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #51      +/-   ##
===========================================
+ Coverage    94.50%   96.75%   +2.25%     
===========================================
  Files            6        7       +1     
  Lines          200      216      +16     
===========================================
+ Hits           189      209      +20     
+ Misses          11        7       -4     
Impacted Files Coverage Δ
openmc_plasma_source/fuel_types.py 100.00% <100.00%> (ø)
openmc_plasma_source/point_source.py 100.00% <100.00%> (+13.33%) ⬆️
openmc_plasma_source/ring_source.py 100.00% <100.00%> (+11.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e430db5...4c80c3d. Read the comment docs.

@shimwell shimwell self-assigned this Feb 9, 2022
@shimwell
Copy link
Member

shimwell commented Feb 9, 2022

Thanks @LiamPattinson this PR looks great to me, I like the separation of the fuel part and the reuse of this class in both the ring source and point source. As you mention this will help if #37 goes ahead.

I think we can merge this unless there are any objections @RemDelaporteMathurin

I have a slight related question on proper_tea as a dependency.

@shimwell shimwell self-requested a review February 9, 2022 11:17
Copy link
Member

@shimwell shimwell 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, happy to merge unless there are any objections

@RemDelaporteMathurin RemDelaporteMathurin merged commit 17e20b7 into fusion-energy:develop Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants