New Options class #33

johnomotani · 2020-05-01T00:26:04Z

Implements a new Options class, removing the dependency on the options package. Options objects are immutable and created by an OptionsFactory. WithMeta utility class stores options along with (optionally) doc, allowed type, list of allowed values, or list of checks that a value must pass. The doc attributes are provided as tool-tips in the table in the GUI.

Closes Options class replacement #27
Tests added
Updated doc/whats-new.md with a summary of the changes

These provide immutable options objects, created by a factory that allows setting sensible defaults (which may depend on the values of other options), checking the type, and whether the value of an option is in an allowed list or passes some tests.

...except for the actual grid point positions, which can be updated with Mesh.redistributePoints().

Using a string with the name of another option is a supported way of giving defaults, and easier to read than lambda expressions.

Now replaced by hypnotoad.utils.options

bendudson · 2020-05-01T09:08:37Z

hypnotoad/core/equilibrium.py

-        if hasattr(self, "regions"):
-            for region in self.regions.values():
-                region.setupSpacing()
+    def resetNonorthogonalOptions(self, nonorthogonal_settings):


I'd prefer to create a new object if possible, rather than reset the options, in case this object is shared elsewhere. Is modifying options definitely needed?

I think it is necessary - once I update #26 you'll be able to try this out, but when creating nonorthogonal grids, the initial construction (creating FineContours and finding wall intersections) takes a long time. Then often the grids look weird for one reason or another, and the way to fix that is to adjust the poloidal spacing of the grid points, which can be done much more quickly than building the Mesh object in the first place, because the FineContours don't need to change, so all the work done in constructing them can just be reused. It does make the workflow a lot better, so I think it's worth the compromise of allowing a small number of options to be changed. I've tried to make sure that there is no persistent state in the Mesh/Equilibrium/MeshRegion/EquilibriumRegion that depends on nonorthogonal_options except for the actual position of the grid points to minimise the chances of some odd history-dependant thing happening. Would still recommend building the grid once from the ground up with the final settings once they're decided on to be safe (and maybe we should enforce that in the GUI at some point).

bendudson · 2020-05-01T09:10:15Z

hypnotoad/cases/tokamak.py

+            5,
+            doc="Number of radial points in the core",
+            value_type=int,
+            checks=is_positive,


nice! I like this very much

hypnotoad/cases/torpex.py

hypnotoad/cases/tokamak.py

hypnotoad/core/equilibrium.py

ZedThree · 2020-05-01T09:00:10Z

hypnotoad/utils/options.py

+        allowed : value or sequence of values, optional
+            When the option is set, it must have one of these values.
+            Cannot be set if 'checks' is given.
+        checks : expression or sequence of expressions, optional
+            When a value is set for this option, all the expressions must return True
+            when called with that value.
+            Cannot be set if 'allowed' is given.


Could combine these two? If allowed is passed a callable then call it, otherwise compare to value? Internally, one way to handle these could be to convert a value into a lambda that does a comparison, and then all the checks would be handled the same. Could then do things like allowed=[None, is_positive] to replace checks=is_positive_or_None

A disadvantage is it might be harder to use that in a GUI interface, whereas allowed values can be used to set the type of widget; checks can be run in the gui to e.g make the widget red when the setting is invalid.

This is true. It looks like so far, the more complicated options are of the form None or <type>, which could be implemented in the gui as an enabling checkbox and the appropriate widget. Something like str or int would be tricky though.

Currently, the gui just takes everything as a str and calls literal_ast.eval on it, so we could implement the checking in the gui straight away. Looks like at the mo it raises an exception

I did have allowed and checks combined as a single option at one point, but split them on the basis that when they're separated if allowed is not None then the GUI can make a drop-down list of the values, which would be more complicated if you have to test whether there is an expression somewhere in allowed.

checks handles a list already, but it requires all the tests to pass, so that you could do something like checks=[is_positive, lambda x: x < 1.] if you wanted to restrict an option to be between 0 and 1. I prefer the way checks is now because it provides more capability, but I don't think I actually use it anywhere at the moment, so it would be possible to change to 'at least one check passes' and then be able to write things like checks=[is_positive, is_None] instead of needing is_positive_or_None.

Ok that seems reasonable!

Future direction might be to split checks into any_of and all_of? It might still be nice to allow values in there, so that any_of=[None, is_positive] works

hypnotoad/utils/options.py

ZedThree · 2020-05-01T09:10:25Z

hypnotoad/utils/options.py

+        # Clean up state of the factory
+        del self.__values


Does this need to be a member-variable in the first place?

Alternatives welcome, but the reason I did this is so that OptionsFactory can implement __getitem__() and __getattr__() methods - this is needed so that the default values can be set (in WithMeta) with expressions like lambda options: options.a + options.b, where the OptionsFactory passes itself as the argument to the expressions. So OptionsFactory.__getitem__(self, key) needs __values without it being passed as an argument.

Maybe one alternative would be to have some kind of dedicated internal class for a temporary mutable object that could be given the __default_values and values and go through evaluating everything, rather than using the OptionsFactory object itself for that?

hypnotoad/core/equilibrium.py

ZedThree · 2020-05-01T09:22:34Z

Just had a play with the GUI. The tooltips are nice! One dull comment: inconsistent capitalisation 😄

And one annoying thing that's Qt's fault: tooltips don't get line-wrapped, and some of them are twice the width of the GUI 🙁 Looks like there's been an open bug for several years.
Here's one possible fix: https://stackoverflow.com/a/46212292/2043465

bendudson · 2020-05-01T09:25:53Z

hypnotoad/utils/options.py

+                factory = OptionsFactory(
+                    a=1,
+                    b=lambda options: options.c + options.a
+                    c=lambda options: 3*options["a"]


nice! I think this is a nice way around e.g. the psi / psinorm problem

bendudson · 2020-05-01T09:30:29Z

This looks good, thanks @johnomotani ! I was skeptical that both factory and Options classes were needed, but the I think the solution is nice and covers a lot of quite complicated use cases in an intuitive way. This should definitely replace Options in PyPI!

Rename torpex.py's withDefault to with_default() and move into utils/options.py so it can be also used in tokamak.py to replace setDefault().

Default arguments are persistent, and the empty dict could potentially be modified, so is a bad design pattern to use {}. Better to use None as the default argument, and handle where necessary.

Requires slightly more typing in the input yaml file, but don't have to split string in the code. Also add check that all items in the refine_methods list are valid in the user_options_factory.

johnomotani · 2020-05-01T12:31:07Z

And one annoying thing that's Qt's fault: tooltips don't get line-wrapped, and some of them are twice the width of the GUI slightly_frowning_face Looks like there's been an open bug for several years.
Here's one possible fix: https://stackoverflow.com/a/46212292/2043465

I tried to apply that fix on this branch https://github.com/boutproject/hypnotoad/tree/new-Options-tooltip-fix but it doesn't seem to do anything - maybe it only wraps if the tooltip reaches the width of the whole screen? I'm not totally sure what the copyright is for a code snippet copied from stackoverflow, so if it's not really helping I'd rather leave it out...

shiftedmetric is used by MeshRegion, not just BoutMesh.

ZedThree · 2020-05-01T14:13:46Z

That snippet failed in multiple different ways for me! Sorry to send you down the wrong path. I found a much nicer solution in the standard library: https://docs.python.org/3.8/library/textwrap.html#textwrap.fill

johnomotani · 2020-05-01T15:53:54Z

I found a much nicer solution in the standard library: https://docs.python.org/3.8/library/textwrap.html#textwrap.fill

Thanks! that looks much simpler :-)

Edit: the tool-tips are much easier to read now. Thanks @ZedThree!

These do not display when using textwrap.fill() to format the tool-tips.

During refactoring all settings were transferred either to user_options_factory or replaced by member variables.

OptionsFactory ended up being written as a general Python utility for handling user options. It has now been packaged as one, and will be maintained at github.com/johnomotani/optionsfactory

sin_angle_at_start should be used to calculate perp_d_lower when 'self.wallSurfaceAtStart is None' instead of when 'is not None', and sin_angle_at_end should be used to calculate perp_d_lower when 'self.wallSurfaceAtEnd is None' instead of when 'is not None'. Error introduced during refactoring in ef0bcdf.

When checking types of options, put 'int' in the allowed types for float variables, so that an exception is not raised if a user happens to pass an int.

ZedThree

🎉 Thanks @johnomotani ! This is definitely a lot cleaner than options

johnomotani added 9 commits April 29, 2020 22:26

Use new Options/OptionsFactory in hypnotoad

ef0bcdf

Ensure no persistent state is set from nonorthogonal_options

5c6a68e

...except for the actual grid point positions, which can be updated with Mesh.redistributePoints().

Add methods to reset nonorthogonal options

0ca3b13

Implement new options in GUI and executable scripts

d3642e8

Use strings instead of expressions for defaults where possible

ca70c28

Using a string with the name of another option is a supported way of giving defaults, and easier to read than lambda expressions.

Remove hypnotoad_options.py

a8b4d69

Remove options package from requirements

03142d7

Now replaced by hypnotoad.utils.options

Update whats-new.md

2809a34

bendudson reviewed May 1, 2020

View reviewed changes

ZedThree reviewed May 1, 2020

View reviewed changes

bendudson reviewed May 1, 2020

View reviewed changes

hypnotoad/core/equilibrium.py Outdated Show resolved Hide resolved

bendudson reviewed May 1, 2020

View reviewed changes

johnomotani added 5 commits May 1, 2020 12:37

Tidy up function that returns default if value is None

defdd59

Rename torpex.py's withDefault to with_default() and move into utils/options.py so it can be also used in tokamak.py to replace setDefault().

Remove empty dict {} default arguments

953b19b

Default arguments are persistent, and the empty dict could potentially be modified, so is a bad design pattern to use {}. Better to use None as the default argument, and handle where necessary.

Use a list for refine_methods rather than comma-separated string

c7f05c5

Requires slightly more typing in the input yaml file, but don't have to split string in the code. Also add check that all items in the refine_methods list are valid in the user_options_factory.

Use a hasattr() check instead of try...except

5d29b4a

Better formatting for options doc attributes

ac32660

Move shiftedmetric to MeshRegion options

36cf3c7

shiftedmetric is used by MeshRegion, not just BoutMesh.

johnomotani added 5 commits May 1, 2020 16:56

Wrap text in tooltips for options widget table

968efd7

Remove line breaks from refine_methods doc

33ba0bb

These do not display when using textwrap.fill() to format the tool-tips.

Remove unused 'options_factory' objects

cce1c15

During refactoring all settings were transferred either to user_options_factory or replaced by member variables.

Move with_default() to utils/utils.py, add tests

052f51d

Replace OptionsFactory with external optionsfactory package

ea78bb8

OptionsFactory ended up being written as a general Python utility for handling user options. It has now been packaged as one, and will be maintained at github.com/johnomotani/optionsfactory

Update minor changes to OptionsFactory API

2369638

johnomotani force-pushed the new-Options branch from fcab586 to 2369638 Compare May 4, 2020 09:41

johnomotani added 3 commits May 4, 2020 14:29

Allow ints to be passed for floats

a3782a6

When checking types of options, put 'int' in the allowed types for float variables, so that an exception is not raised if a user happens to pass an int.

Fix SolutionError message in refinePoint()

ca005f1

johnomotani mentioned this pull request May 5, 2020

Tokamak nonorthogonal grids #26

Merged

1 task

johnomotani added this to the Release 0.2 milestone May 5, 2020

ZedThree approved these changes May 5, 2020

View reviewed changes

johnomotani merged commit 12af9e1 into master May 5, 2020

johnomotani deleted the new-Options branch May 5, 2020 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Options class #33

New Options class #33

johnomotani commented May 1, 2020 •

edited

Loading

bendudson May 1, 2020

johnomotani May 1, 2020

bendudson May 1, 2020

ZedThree May 1, 2020

bendudson May 1, 2020

ZedThree May 1, 2020

johnomotani May 1, 2020

ZedThree May 1, 2020

ZedThree May 1, 2020

johnomotani May 1, 2020

johnomotani May 1, 2020

ZedThree commented May 1, 2020

bendudson May 1, 2020

bendudson commented May 1, 2020

johnomotani commented May 1, 2020

ZedThree commented May 1, 2020

johnomotani commented May 1, 2020 •

edited

Loading

ZedThree left a comment

New Options class #33

New Options class #33

Conversation

johnomotani commented May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZedThree commented May 1, 2020

Choose a reason for hiding this comment

bendudson commented May 1, 2020

johnomotani commented May 1, 2020

ZedThree commented May 1, 2020

johnomotani commented May 1, 2020 • edited Loading

ZedThree left a comment

Choose a reason for hiding this comment

johnomotani commented May 1, 2020 •

edited

Loading

johnomotani commented May 1, 2020 •

edited

Loading