fsciortino / Aurora

Modern toolbox for impurity transport, neutrals and radiation modeling in magnetically-confined plasmas
https://aurora-fusion.readthedocs.io
MIT License
39 stars 22 forks source link

Support using a custom normalisation with oedge_case.plot_2d #68

Closed CorvidCanine closed 1 year ago

CorvidCanine commented 1 year ago

It is useful to have the option of using a custom normalisation with plot_2d in the oedge_case class, instead of one of the internally calculated options. Particularly if you need to sync the normalisation of the colour-bars of multiple plots.

This adds the new kwarg norm to plot_2d. When the normtype is set to "passed" then any Normalize object passed through norm will be used for plotting instead of calculating one. A TypeError is raised if the normtype is set to passed but norm is not a subclass of mpl.colors.Normalize. Such as leaving it set to the default None.

This request also adds throwing a ValueError when normtype is a string that is not one of the existing calculated or the passed options.

fsciortino commented 1 year ago

Thanks for the PR, @CorvidCanine ! May I suggest to condense the arguments, so as to have only norm, as opposed to both norm and normtype? One could do something like

if isinstance(norm, mpl.colors.Normalize):
    pass
elif isinstance(norm, str):
    # previous options
else:
    raise ValueError("Unrecognized 'norm' option: it must be either an mpl.colors.Normalize instance or a string.")
CorvidCanine commented 1 year ago

Combining the two arguments makes sense, it does look clearer this way. The change is now breaking though, as existing code that tries to pass the old keyword argument normtype will throw an error.

fsciortino commented 1 year ago

Thanks @CorvidCanine, looks great! I'm not worried about backward-compatibility at this stage because the oedge capabilities are still relatively new and I know pretty well who is using them at this stage and this won't be a problem. Thanks for considering such issues though (now and in the future)!