fusion-energy / openmc-plasma-source

Creates a plasma source as an openmc.source object from input parameters that describe the plasma
MIT License
26 stars 11 forks source link

Made TokamakSource automatic. #16

Closed mdfaisal98 closed 2 years ago

mdfaisal98 commented 2 years ago

This PR makes TokamakSource more automatic by defining it in the __init__() method, solving #13.

RemDelaporteMathurin commented 2 years ago

@mdfaisal98 thanks for working on this. A few things:

shimwell commented 2 years ago

Oh it looks like FusionRingSource now requires the argument radius and the test doesn't have that argument, this is somewhat unrelated to the aim of the PR.

RemDelaporteMathurin commented 2 years ago

Oh it looks like FusionRingSource now requires the argument radius and the test doesn't have that argument, this is somewhat unrelated to the aim of the PR.

Ah yes it is related to #14

@mdfaisal98 you need to update your main branch which doesn't have the latest changes

https://stefanbauer.me/articles/how-to-keep-your-git-fork-up-to-date

This link explains how to keep your forks up to date: this is essential when it comes to contributions

shimwell commented 2 years ago

There is now a "fetch upstream" button on forks that can just be clicked. It is towards the upper right corner

mdfaisal98 commented 2 years ago

I think the issue was due to the fact that I had made my changes in mdfaisal98:main which didn't have the test changes of the branch develop. Should I close this PR and create a new one? @RemDelaporteMathurin

RemDelaporteMathurin commented 2 years ago

@mdfaisal98 by fetching the upstream (syncing your fork with this repo) and then pushing the changes to your main, this PR should update automatically, no need to close it.

Although if you prefer to start fresh you can of course :-)

codecov[bot] commented 2 years ago

Codecov Report

Merging #16 (3e4c271) into develop (530fae2) will increase coverage by 0.23%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #16      +/-   ##
===========================================
+ Coverage    82.75%   82.99%   +0.23%     
===========================================
  Files            4        4              
  Lines          145      147       +2     
===========================================
+ Hits           120      122       +2     
  Misses          25       25              
Impacted Files Coverage Δ
openmc_plasma_source/tokamak_source.py 84.82% <100.00%> (+0.27%) :arrow_up:

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 530fae2...3e4c271. Read the comment docs.

RemDelaporteMathurin commented 2 years ago

The tests seem to pass and this seems ready to merge!

Thanks @mdfaisal98 for this effort! 🎉

mdfaisal98 commented 2 years ago

@mdfaisal98 by fetching the upstream (syncing your fork with this repo) and then pushing the changes to your main, this PR should update automatically, no need to close it.

Although if you prefer to start fresh you can of course :-)

I have made changes. Thanks for the help @RemDelaporteMathurin @Shimwell