Skip to content
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

Remove optparse from example #655

Merged
merged 2 commits into from
Apr 19, 2021
Merged

Remove optparse from example #655

merged 2 commits into from
Apr 19, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Apr 12, 2021

closes #607

This PR removes optparse in https://github.com/enthought/chaco/blob/ef7227706642f7d26042717ad9b60648d7572068/examples/demo/advanced/scalar_image_function_inspector.py and also fixes the following error:

$ python examples/demo/advanced/scalar_image_function_inspector.py -n 50
2021-04-12 16:29:40.435 Python[68324:22123757] ApplePersistenceIgnoreState: Existing state will not be touched. New state will be written to (null)
Exception occurred in traits notification handler for object: <__main__.PlotUI object at 0x7fee44635200>, trait: num_levels, old value: 15, new value: 50
Traceback (most recent call last):
  File "/Users/aayres/.edm/envs/chaco-test-3.6-pyqt5/lib/python3.6/site-packages/traits/trait_notifiers.py", line 342, in __call__
    self.handler(*args)
  File "examples/demo/advanced/scalar_image_function_inspector.py", line 452, in _num_levels_changed
    self.polyplot.levels = self.num_levels
AttributeError: 'NoneType' object has no attribute 'levels'

by adding an is not None check to the example.

All of the options for optparse are configurable within the application itself and so it was decided the command line options were unnecessary

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I take it back. If we want to contribute this to etsdemo, it will be best if we dont introduce the additional dependency on click. Can you rewrite this to use argparse instead? Sorry for the double work.

@aaronayres35
Copy link
Contributor Author

Sorry. I take it back. If we want to contribute this to etsdemo, it will be best if we dont introduce the additional dependency on click. Can you rewrite this to use argparse instead? Sorry for the double work.

No worries at all. I'm happy to make the switch but before I do I wanted to ask: do we event want to support command line arguments at all in that case? All the options (function, nlevels, and colormap) are configurable via the UI. I don't exactly see the need of having the command line options, plus it clouds the example with non-chaco code / one is probably looking at the example to learn about chaco not click or aragparse.

However, if we do want the functionality I am happy to just make the switch!

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Can you add _default methods for function, num_levels and colormap? It looks like the changes in _num_levels_changed are motivated by the lack of good default values for those traits.

Also, can you update the PR title and description based on the decision we made?

@aaronayres35 aaronayres35 changed the title Replace optparse with click in example Remove optparse from example Apr 19, 2021
@aaronayres35
Copy link
Contributor Author

Can you add _default methods for function, num_levels and colormap? It looks like the changes in _num_levels_changed are motivated by the lack of good default values for those traits.

function already defaults to the default function as its trait definition is function = Str("tanh(x**2+y)*cos(y)*jn(0,x+y*2)"). Likewise for num_levels = Int(15). colormap is a little weird, as _cmap defaults to viridis as desired and that is what is actually used. colormap defaults to whatever happens to be the first item in list(default_colormaps.color_map_name_dict.keys()), but when it changes _cmap is updated as needed.
The changes in _num_levels_changed have to do with the default for polyplot and lineplot which get created in create_plot. I think changing this may be a bit more complicated, but I am happy to look into it. It might be better to do that in a follow up PR though IMO

Also, can you update the PR title and description based on the decision we made?

Yep, done

@rahulporuri
Copy link
Contributor

Still LGTM

@aaronayres35 aaronayres35 merged commit 4b3747d into master Apr 19, 2021
@aaronayres35 aaronayres35 deleted the replace-optparse branch April 19, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace optparse with argparse in example
2 participants