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

Removed proper_tea, replaced with param #56

Closed LiamPattinson closed 2 years ago

LiamPattinson commented 2 years ago

I switched out proper_tea for the much more widely used package param. It supports much more than what my own library can do, though I personally think my version is more readable and easier to use.

Advantages of param:

Disadvantages of param:

codecov[bot] commented 2 years ago

Codecov Report

Merging #56 (7ab40af) into develop (2d27eb7) will decrease coverage by 0.05%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #56      +/-   ##
===========================================
- Coverage    96.34%   96.29%   -0.06%     
===========================================
  Files            7        7              
  Lines          219      216       -3     
===========================================
- Hits           211      208       -3     
  Misses           8        8              
Impacted Files Coverage Δ
openmc_plasma_source/fuel_types.py 100.00% <100.00%> (ø)
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.19% <100.00%> (-0.03%) :arrow_down:

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 2d27eb7...7ab40af. Read the comment docs.

LiamPattinson commented 2 years ago

Another disadvantage of param is that it's quite picky with regards to some input types. For example, the NumericTuple and Range parameters must be given as a tuple, while my implementation would quite happily take any iterable.

shimwell commented 2 years ago

Thanks Liam, I think this is the best way to go as well, I'm sad to lose proper_tea which has one more advantage (best name).

I can go through this PR tonight, but it all looks great on an initial skim

LiamPattinson commented 2 years ago

Nicely spotted, I'd assumed Param was using default=None, so didn't think setting it to None explicitly would change anything. It seems that instead it's using default=0, which fails when bounds are chosen that exclude zero.

I'm now failing the tests because it can't find openmc, but I didn't change any of the import lines since my last commit. Any idea what could be causing this?

RemDelaporteMathurin commented 2 years ago

Interesting... it uses openmc:latest and I think there was a release yesterday in openmc. Maybe something was messed up in the docker image. What happens if you try a previous tag of the openmc image?

shimwell commented 2 years ago

That sounds like something has changed inside the openmc docker image, I can look into this

shimwell commented 2 years ago

I've reproduced the error, this will need fixing at the openmc end

Screenshot from 2022-02-15 15-04-02

shimwell commented 2 years ago

Perhaps we can target a different docker image for now

openmc/openmc:v0.12.2

RemDelaporteMathurin commented 2 years ago

Perhaps we can target a different docker image for now

openmc/openmc:v0.12.2

Yep, another reason to use fixed version tags for dependencies....... :(

LiamPattinson commented 2 years ago

Apologies, it looks like I've just pushed the exact same changes as those in pull request https://github.com/fusion-energy/openmc-plasma-source/pull/57.

RemDelaporteMathurin commented 2 years ago

@LiamPattinson ah sorry you were faster than me. There are some conflicts now but the tests pass :-)

shimwell commented 2 years ago

LGTM