search

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

Add openmc_source_plotter features #41

Closed RemDelaporteMathurin closed 2 years ago

RemDelaporteMathurin commented 2 years ago

@shimwell what would you think of adding to ops some plotting features by importing openmc_source_plotter?

I wonder if the packages should be merged. I guess the question is: "Would the openmc_source_plotter be used outside of fusion applications?" If the name of this org is fusion-energy, i think the answer's no. Maybe these two packages could be merged to reduce the number of dependencies.

What do you think?

shimwell commented 2 years ago

openmc_source_plotter could be used by fisson (eigenvalues) OpenMC sources. I believe there are more fission users of OpenMC than (fix source) fusion users. So I wouldn't suggest merging but we could certainly import openmc_source_plotter into openmc-plasma-source if that helps.

Also it might be worth waiting a little longer as openmc_source_plotter package currently only works with openmc version 0.11 for fix source and should work with the current 0.12 and 0.13(dev) version of openmc for eigenvalues.

In about two months from now the OpenMC sampling should be exposed via the openmc.lib python interface and then openmc_source_plotter should work with the latest version of openmc again.

In general the packages on fusion-energy at split by topic and the dependencies stack. Currently the dependencies make the combination of these two packages problematic. Can we come back to this idea in a couple of months

RemDelaporteMathurin commented 2 years ago

openmc_source_plotter could be used by fisson (eigenvalues) OpenMC sources

Well in that case should it live in "fusion-energy"?

we could certainly import openmc_source_plotter into openmc-plasma-source if that helps

Yes that would be an easy way to do it.

shimwell commented 2 years ago

Well in that case should it live in "fusion-energy"?

If it becomes a dependency of openmc-plasma-source then it makes life easier if it stays in the same organization where we can keep them both working together nicely and efficiently (PRs easy to merge here and releases easy to make as well). It is currently not working well enough to offer it to the openmc org. It could go to fusion-neutronics if you like. Were would you like to relocate the repo ideally?

RemDelaporteMathurin commented 2 years ago

I don't know. But I just feel that:

1) if the purpose of the package is fusion applications, then it could be merged with ops since these two packages have the same goal: making life easier for openmc users that model fusion applications 2) if it's a package for fusion AND fission, it doesn't belong in an org called "fusion-energy"

I agree with you that keeping it in the fusion-energy would facilitate things, so I would suggest option 1) (merging it with ops). Maybe after some time if the api is stable enough we can offer it to openmc.

I believe we need to find a trade-off between "all features in a single mega package" and "too many different packages over-specialised". ops and openmc_source_plotter have really similar goals: facilitate the making of openmc.source for fusion (cause we are in fusion-energy). They could be merged :-)

shimwell commented 2 years ago

Yep totally agree we need to find a trade off, here is some justification for the current balance.

I'm not sure the disciplines are as clear cut as that, there are lots of things that are useful for fission and fusion but live in one camp more that the others. They both have neutrons after all so there is tons of overlap.

I think the packages have a different remit. One is creating sources, one is visualisation sources. Visualisation is often a post process. Creation is pre simulation and doesn't nessecary require visualisation.

The dependency stack is different, the plotting requires plotly (and all that plotly depends on) and in the future vtk and other such packages. This doesn't sound difficult for Linux desktop users but:

In addition to making life more difficult for users of the above systems merging it would also cut of several use cases and potential users:

Currently the plan is to allow any OpenMC source to be plotted with the plotter. User who want to plot a source will find it via Google or pypi by its relevant name. They will be generating plots quickly as there are only three functions to understand. They can install it in most places easily due to a minimal dependency stack. They are unlikely to find the plotting package if it is buried inside a plasma source package. They are less likely to find the plotting features due to the larger amount of features to search through. They are less likely to be able to install it due to extra packages in the dependency stack.

Maintenance / testing burden also doesn't scale well as package sizes increase.

So I'm open to merging, importing, moving the packages but there are some reasons why it is set out in the way it is.

Also the plotter package doesn't work with the current version of OpenMC and can only plot 1 out of the 3 plasma sources available, so best to get it working first 😜. Can we come back to this once OpenMC.lib supports source sampling. It will change the plotting package quite a lot.

RemDelaporteMathurin commented 2 years ago

@shimwell based on our conversation I understand the need of plotly. So maybe it would have to be done the other way arround: add support for plotly visualisation of TokamakSource in openmc-plasma-source via importing openmc_source_plotter.

linked to #47 you could have:

my_source = TokamakSource(....)

my_source.plot_3d(backend="matplotlib")
my_source.plot_3d(backend="plotly")

here the method plot_3d would import opp and do all the job behind the scenes

shimwell commented 2 years ago

Looks like a good plan to me. opp can't do ring sources quite yet as they were not in openmc v0.11 so this might have to wait for the openmc.lib upgrade

py1sl commented 2 years ago

Please leave the source plotter separate, plasma sources are not the only sources used in fusion, e.g. activation sources or derived sources for streaming calculations where plotting the direction vectors is helpful

shimwell commented 2 years ago

This PR refactors the openmc-source-plotter to use the new openmc.lib sampling feature.

https://github.com/fusion-energy/openmc_source_plotter/pull/13

I think this would be a reasonable starting point for plotly and matplotlib plots

shimwell commented 2 years ago

Took me a while but I've revamped the openmc-source-plotter package and then proposed we make use of it on this package in PR #63

I think the plotting side of things is now easy to improve over on openmc-source-plotter and we make use of it here.

Currently the plotting just supports plotly, but mpl could be added

RemDelaporteMathurin commented 2 years ago

I kinda liked the old openmc-source-plotter way of doing things with functions rather than an actual class.

This means that now everytime someone makes a new source, it will have to inherit from SourceWithPlotting in order to have these plotting functionalities. Whereas with a function, it could work with any openmc.Source object.

I totally understood yours and @py1sl 's point of view in not merging openmc source plotter with this tool and I'm honestly happy in keeping them appart.

I just feel that if we start inheriting all the classes from this new SourceWithPlotting then there's no end to it.

But then again, if this is how openmc source plotter now works, I guess we don't have a choice 😄

shimwell commented 2 years ago

Closing this now as it is now possible to plot these sources and any other openmc sources with the openmc_source_plotter package. There is no particular need to bring that functionality into to this package as discussed in PR #66 and #63 .