-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Options class #33
Conversation
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
if hasattr(self, "regions"): | ||
for region in self.regions.values(): | ||
region.setupSpacing() | ||
def resetNonorthogonalOptions(self, nonorthogonal_settings): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 FineContour
s 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 FineContour
s 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).
hypnotoad/cases/tokamak.py
Outdated
5, | ||
doc="Number of radial points in the core", | ||
value_type=int, | ||
checks=is_positive, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I like this very much
hypnotoad/utils/options.py
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
# Clean up state of the factory | ||
del self.__values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a member-variable in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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. |
hypnotoad/utils/options.py
Outdated
factory = OptionsFactory( | ||
a=1, | ||
b=lambda options: options.c + options.a | ||
c=lambda options: 3*options["a"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I think this is a nice way around e.g. the psi / psinorm problem
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.
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.
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 |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks @johnomotani ! This is definitely a lot cleaner than options
Implements a new
Options
class, removing the dependency on theoptions
package.Options
objects are immutable and created by anOptionsFactory
.WithMeta
utility class stores options along with (optionally)doc
, allowed type, list of allowed values, or list of checks that a value must pass. Thedoc
attributes are provided as tool-tips in the table in the GUI.doc/whats-new.md
with a summary of the changes