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

plotting functions should be methods of TokamakSource #47

Open
RemDelaporteMathurin opened this issue Feb 1, 2022 · 5 comments
Open
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@RemDelaporteMathurin
Copy link
Member

RemDelaporteMathurin commented Feb 1, 2022

I was looking at the functions in plotting and thought these should maybe be methods of TokamakSource so that users can do:

my_source = TokamakSouce(....)
my_source.plot_3d(...)
@LiamPattinson
Copy link
Contributor

I tend to be of the opinion that objects shouldn't know how to plot themselves. The user may have no intention of plotting TokamakSource, but making those methods part of the class means they now must take on another non-optional dependency. It can also cause class bloat, and as the project grows the amount of responsibilities held by one class can get out of hand.

Then again, the syntax is quite nice. Perhaps something like this would work?

def plot_3D(self,*args,**kwargs):
    from .plotting import plot_tokamak_source_3D
    plot_tokamak_source_3D(self,*args,**kwargs)

It permits the use of either method, and keeps class bloat to a minimum, though I'm generally not a fan of importing things anywhere except the top of a file. I think it might be considered a PEP8 issue, and it can lead to frustrating errors. However, it does work...

@shimwell
Copy link
Member

shimwell commented Feb 11, 2022

Once the openmc.lib has access to the source sampling the I plan to update this package so that it can sample a source without making an initial_source.h5.

This is a longer term plan but I hope to make openmc_source_plotter into a nice source plotting package for any openmc source

@RemDelaporteMathurin
Copy link
Member Author

so that it can sample a source without making an initial_source.h5.

Hm... ops doesn't create initialèsource.h5 does it?

@shimwell
Copy link
Member

ops (openmc-plasma-source) has a plotting function for the tokamak source but it doesn't make use of the openmc distributions to plot. It doesn't actually plot the source as openmc sees it.

Therefore ops doesn't create an initial_source.h5 as it doesn't make use of openmc to sample energy, position or direction.

ops doesn't plot the actual positions, energies or directions of neutrons that are created in the neutronics code.

So unless we want to program in every distribution that OpenMC understands I think we should make use of OpenMC to sources sampling routines to get the source details.

This inital_source feature is only in version 0.11 for fixed sources and unfortunate version 0.11 doesn't include the ring source. So that is why I suggest we wait for openmc.lib to support source sampling.

@RemDelaporteMathurin
Copy link
Member Author

Gotcha sorry I thought you meant that currently ops ued initial_source.h5 but I must've gotten mixed up.

It would be so much better if we could also plot the source as openmc sees it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants