pyro-kinetics / pyrokinetics

Python library to run and analyse gyrokinetics simulations
https://pyrokinetics.readthedocs.io/en/latest/#
GNU Lesser General Public License v3.0
25 stars 6 forks source link

Use explicit `__init__` for `LocalGeometry` #384

Open LiamPattinson opened 2 months ago

LiamPattinson commented 2 months ago

Resolves (partly) https://github.com/pyro-kinetics/pyrokinetics/issues/336.

Replaces general __init__ functions of the form def __init__(self, *args, **kwargs): with more explicit argument lists. Uses the same default arguments as are currently in use.


TODO:

Stretch goals:

LiamPattinson commented 2 months ago

The method from_global_eq is now a @classmethod, meaning it can be used as an alternative constructor in place of the basic __init__, and can no longer be used to modify an existing LocalGeometry.

[!Note] The ability to set pyro.local_geometry with a string has been removed. Rather than converting the currently loaded LocalGeometry to the desired type, this would instead replace it with an empty object, which risks allowing uninitialised data to enter the program.

This method was only used in the function load_local_geometry to pre-emptively set the type of LocalGeometry before updating it with from_global_eq. With from_global_eq being a classmethod, this is no longer needed.

This method was also used in some examples scripts, but they were easily modified to use a safer method.

(This also allowed me to delete a 2 year old FIXME comment I'd left in Pyro, which I'm quite happy about)


Also fixed a minor error in the Equilibrium docs while I was at it

LiamPattinson commented 2 months ago

The latest commit changes from_local_geometry to be a @classmethod:

# Before
local_geometry = LocalGeometryMXH() # Empty object
local_geometry.from_local_geometry(other_local_geometry)

# After
local_geometry = LocalGeometryMXH.from_local_geometry(other_local_geometry)

From here, I could either carry on working on this to complete the additional goals listed above, or (if you're happy with the changes) we could merge this and I'll raise a new PR once I've made some progress. I'm hoping to be done with the further goals roughly by the end of next week.

bpatel2107 commented 2 months ago

This looks great, makes LocalGeometry more like the other classes and I think being consistent overall is a good idea. Making it a @dataclass seems likea good way to go ahead.

normalise adds/converts the units of the different LocalGeometry parameters. When all is said and done they should have units. The issue becomes whether or not the units or way to convert units is known when initialising the object. When loading from Equilibrium it is straightforward, but when loading from input files there may be times where the units are not immediately obvious without looking at other parts of the input file. This could be handled in the specific code such that from_gk_data includes the appropriate units/convention (should be the called load_from_gk_data?). So normalise should be in the __init__ but we need to make sure we know the appropriate units.

LiamPattinson commented 1 month ago

With the aim of promoting immutability, normalise() now returns a new object, and from_global_eq no longer updates the current normalisation as a side effect. In order to merge LocalGeometry and FluxSurface, it would be ideal to remove the norms argument from from_global_eq entirely, so I'll have a think about how best to achieve that. I also still have to fix a couple of broken tests and go over the docs.

LiamPattinson commented 1 month ago

I think that's about as far as I'd like to take this PR. I'd still like to merge FluxSurface and LocalGeometry, but it's less essential. I've reworked the docs, removed most default __init__ parameters, and fixed the broken tests. I also removed the norms argument from from_global_eq, as if I do get around to merging FluxSurface and LocalGeometry, it would be good to be able to do so without creating a Normalisation first.