search

fusion-energy / openmc-plasma-source

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

Using openmc source plotter for plotting methods #66

Closed shimwell closed 2 years ago

shimwell commented 2 years ago

This PR is a rework of #63 with feedback taken onboard.

The source class is now openmc.Source and it has been monkey patched to add the desired plotting methods.

This now just looks and behaves like a regular openmc source but it has a few plot methods

codecov[bot] commented 2 years ago

Codecov Report

Merging #66 (17ad993) into develop (a9f29a8) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop      #66      +/-   ##
===========================================
+ Coverage    96.29%   96.34%   +0.05%     
===========================================
  Files            7        7              
  Lines          216      219       +3     
===========================================
+ Hits           208      211       +3     
  Misses           8        8
Impacted Files Coverage Δ
openmc_plasma_source/point_source.py 100.00% <100.00%> (ø)
openmc_plasma_source/ring_source.py 100.00% <100.00%> (ø)
openmc_plasma_source/tokamak_source.py 97.22% <100.00%> (+0.02%) :arrow_up:

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

shimwell commented 2 years ago

tests are now passing here as well :tada:

shimwell commented 2 years ago

The change is quite sneaky, by simply importing openmc_source_plotter the openmc.Source object gains plotting methods. This is done via a monkey patch

RemDelaporteMathurin commented 2 years ago

The change is quite sneaky, by simply importing openmc_source_plotter the openmc.Source object gains plotting methods. This is done via a monkey patch

Ah ok. Not sure this is good practice haha

It is kinda confusing really... What motivated the move in OSP from functions to a class-based architecture? I don't really see the benefits here. It's a mix between having things seperate, and having things bundled in the same package... confusing.

shimwell commented 2 years ago

No worries I can still use this package with the plotter by importing them both in the user script.

The user script for class based is a bit simpler than before IMO

function based

import openmc
import openmc_source_plotter as osp
my_source = openmc.Source()
osp.plot_source_energy(source=my_source)

class based

import openmc
import openmc_source_plotter
my_source = openmc.Source()
my_source.plot_source_energy()
RemDelaporteMathurin commented 2 years ago

The user script for class based is a bit simpler than before IMO

It's not a lot simpler. It requires the users to understand the monkeypatch thing... it is not straightforward (my misunderstanding is a good example haha).

And it creates a discrepency with the openmc documentation... I just don't know if it's worth the trouble.

I think I'd rather have the packages seperated than bundled like that.

shimwell commented 2 years ago

I shall close this PR, but to answer the documentation question I can make documentation in the package to describe the additional 4 methods added to openmc.Source