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

Contribute examples to etsdemo #751

Merged
merged 24 commits into from
Jun 7, 2021
Merged

Contribute examples to etsdemo #751

merged 24 commits into from
Jun 7, 2021

Conversation

aaronayres35
Copy link
Contributor

closes #535

This PR contributes the relevant examples to the etsdemo application and does so by moving them inside the package. I am still in the process of going through the examples and trying to decide what is relevant / useful / functioning correctly.

I also think we may just want to leave the left over examples where they are and maybe rename the folder to legacy_examples or something like that. IIRC we did something like this with envisage. Or if a reviewer prefers they can be deleted.

@rahulporuri rahulporuri self-requested a review June 3, 2021 15:26
@aaronayres35
Copy link
Contributor Author

Currently the documentation does not use literal includes, but the files in examples/tutorials / examples/user_guide do align with the code examples from the docs. Perhaps those should be kept / moved / updated as needed and literal includes used in the docs?

Also once we have a final decision on the examples to include, I will delete those left behind. I have gone through the examples and I think I grabbed the majority that we will want to keep. It is possible some of those left should be kept as well. For example either multiaxis.py or multiaxis_using_Plot.py.

if __name__ == "__main__":
hyetograph = Hyetograph()
hyetograph.start()
demo.start()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not working correctly (the demo works different when run in etsdemo vs standalone). I imagine it is because etsdemo looks for demo and expects to call configure_traits or something, whereas here we really need to call start / we need to call _perform_calculations first...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the example to avoid this problem. Note this example is referenced in the documentation, so perhaps the docs themselves should be changed to match?

@aaronayres35
Copy link
Contributor Author

aaronayres35 commented Jun 3, 2021

qt_example.py is a strange case as etsdemo is intended to work with HasTraits classes specifically, rather than qt itself. This I was unable to get to work with etsdemo, but I feel the example is still worth keeping. I think I will add to the file docstring saying "This demo is intended to be run standalone and will not work within the etsdemo application" or something along those lines. EDIT: done



if __name__ == "__main__":
show_plot()
demo.configure_traits()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would look / work a lot better as a popup, but that leads to very strange errors due to etsdemo internals. Doing so I get the following error:

Traceback (most recent call last):
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traitsui/qt4/instance_editor.py", line 428, in edit_instance
    view, kind=factory.kind, id=factory.id
  File "<string>", line 603, in edit_traits
  File "<string>", line 599, in _start_timer
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traits/trait_types.py", line 3360, in validate
    self.resolve_class(object, name, value)
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traits/trait_types.py", line 3462, in resolve_class
    super().resolve_class(object, name, value)
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traits/trait_types.py", line 3178, in resolve_class
    self.validate_failed(object, name, value)
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traits/trait_types.py", line 3207, in validate_failed
    self.error(object, name, value)
  File "/Users/aayres/.edm/envs/test-chaco/lib/python3.6/site-packages/traits/base_trait_handler.py", line 75, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'model_view' trait of a TimerController instance must be a ModelView or None, but a value of <___main___.ModelView object at 0x7fa7c4a40150> <class '___main___.ModelView'> was specified.
Abort trap: 6

we have a trait defined as a = Instance("B") where B is a class defined elsewhere in the module. However, due to etsdemo messing with locals to change __name__ to ___name___, we end up passing an instance of ___name___.B which traits catches as an invalid value...
I do not know how to resolve this. Perhaps we just leave it as demo embedded in the etsdemo ui for now (it is resizable anyway so if you stretch the etsdemo ui out enough you can see the full demo).

@rahulporuri any ideas on how we could resolve this? It is awkward because you need to do Instance("ModelView") not Instance(ModelView) because ModelView is defined after TimerController in the file, and ModelView also has a trait which is Instance(TimerController) so we can't simply move the class to be defined above.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. if possible, can we rename it to something other than ModelView because that slightly confuses things
  2. I don't think it's bad to use Instance("ModelView") instead of Instance(ModelView). I think there are definitely valid usecases (like here) where a class is defined later in the module which needs to be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note:
switching to

model_view = Instance(
       "chaco.examples.demo.advanced.scalar_image_function_inspector.DemoModelView"  # noqa: E501
   )

did not solve the issue. I will leave it as a demo not a pop up for now

@aaronayres35
Copy link
Contributor Author

Some other demos we may want to have be popup instead of demo as they are a bit too big to fit in the etsdemo applications window. I will go through and audit these and update as needed. Possibly in this PR or I can do it in a quick follow up PR.

@aaronayres35
Copy link
Contributor Author

Also akin to enthought/enable#805 we should look at moving or deleting https://github.com/enthought/chaco/blob/master/chaco/example_support.py

However, currently only enable.example_support is used in the chaco codebase, and as this PR currently stands only 2 examples currently being kept use it. We probably should not be importing this from enable in the first place, but internal to ets maybe that sin can be overlooked.
In any case we should either delete the unused chaco module, or change the uses of enable.example_support inside chaco to use chaco.example_support instead. Also any remaining example_support modules should be moved into their packages examples folder, eg what is done in enable PR 805 linked above

@aaronayres35 aaronayres35 changed the title [WIP] Contribute examples to etsdemo Contribute examples to etsdemo Jun 3, 2021
Comment on lines +6 to +10

* 'left' : index increases left to right
* 'right' : index increases right to left
* 'top' : index increases top to bottom
* 'bottom' : index increases bottom to top
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was previously giving an indentation warning inside the etsdemo app

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.

Image from file is broken with the following error -

FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\rporuri\\.edm\\envs\\chaco-test-3.6-pyside2\\Scripts\\capitol.jpg'

scatter_inspector2 fails with -

NameError: name '_create_plot_component' is not defined

qt_example doesn't show anything - no errors but no visible output in the demo pane either

chaco/examples/demo/advanced/data_cube.py Show resolved Hide resolved
chaco/examples/tests/test_etsdemo_info.py Outdated Show resolved Hide resolved
@@ -1,23 +0,0 @@
# -------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we deleted this file to remove any confusion about how to interact with the examples - the examples are meant to be used via the etsdemo application. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that is correct

@aaronayres35
Copy link
Contributor Author

Image from file is broken with the following error -

FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\rporuri\\.edm\\envs\\chaco-test-3.6-pyside2\\Scripts\\capitol.jpg'

I realize looking at it now that Mark would frown at this example as it does:
from traits.util.resource import find_resource
Traits should not be a place to get general utility functions, and perhaps this example should be rewritten to avoid this? I think I will remove it from this PR entirely, and we can come back and rework it / add it back in the future. What do you think?

@aaronayres35
Copy link
Contributor Author

@rahulporuri
A couple of questions:
Any thoughts on #751 (comment) and #751 (comment)?
Also are there any other examples you think should be moved in and kept? If not I can go ahead and delete those which were not chosen in a final commit before this PR is merged.

@aaronayres35 aaronayres35 requested a review from rahulporuri June 7, 2021 13:34
@rahulporuri
Copy link
Contributor

Also akin to enthought/enable#805 we should look at moving or deleting https://github.com/enthought/chaco/blob/master/chaco/example_support.py

Let's defer that for this release. We already have too much on our plate and dealing with this properly might push things by a day or two.

It is possible some of those left should be kept as well

I want to defer this decision to after the release as well - im sure there are more examples that we can move into the package and maybe some which we can delete outright. Either way, this will need us to sit together for an hour or so after having reviewed all of the examples, which is time we are currently short on i think.

Perhaps those should be kept / moved / updated as needed and literal includes used in the docs?

It'll definitely be a good idea to use literalinclude in the docs instead of duplicating the code. Again, one more thing to defer.

Can you create issues for all of these undecided/incomplete work items so that we can follow up later?

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

@aaronayres35
Copy link
Contributor Author

aaronayres35 commented Jun 7, 2021

Can you create issues for all of these undecided/incomplete work items so that we can follow up later?

yep, will do!

EDIT: done, I've opened #760, #761, and #762

@aaronayres35 aaronayres35 merged commit 4134400 into master Jun 7, 2021
@rahulporuri rahulporuri deleted the contribute-to-etsdemo branch June 7, 2021 13:54
aaronayres35 added a commit that referenced this pull request Jun 8, 2021
* etsdemo boilerplate

* delete demo.py

* move examples/demo/basic into chaco/examples

* move select examples from examples/demo/advanced into chaco/examples

* move examples/demo/financial into chaco/examples

* move a bunch of selected examples from examples/demo into chaco/examples/demo

* typo

* add asynchronous_updates example

* fix accidental placement into entry_points not package_data

* add toolbar_plot and chaco_trait_editor examples

* update tox.ini

* remove mayavi as an examples dep

* define demo variable for use in etsdemo

* update hyetograph so that it works with etsdemo

* update various examples to work within etsdemo application

* add comment that qt_example.py will not work in etsdemo app

* make scalar_image_function_inspector.py work in etsdemo

* make a few other examples work inside etsdemo

* make examples using DemoFrame and demo_main work in etsdemo

* flake8

* Update chaco/examples/tests/test_etsdemo_info.py

Co-authored-by: Poruri Sai Rahul <[email protected]>

* rename ModelView as DemoModelView

* fix scatter_inspector2

* remove todo list

Co-authored-by: Poruri Sai Rahul <[email protected]>
aaronayres35 added a commit that referenced this pull request Jun 8, 2021
* etsdemo boilerplate

* delete demo.py

* move examples/demo/basic into chaco/examples

* move select examples from examples/demo/advanced into chaco/examples

* move examples/demo/financial into chaco/examples

* move a bunch of selected examples from examples/demo into chaco/examples/demo

* typo

* add asynchronous_updates example

* fix accidental placement into entry_points not package_data

* add toolbar_plot and chaco_trait_editor examples

* update tox.ini

* remove mayavi as an examples dep

* define demo variable for use in etsdemo

* update hyetograph so that it works with etsdemo

* update various examples to work within etsdemo application

* add comment that qt_example.py will not work in etsdemo app

* make scalar_image_function_inspector.py work in etsdemo

* make a few other examples work inside etsdemo

* make examples using DemoFrame and demo_main work in etsdemo

* flake8

* Update chaco/examples/tests/test_etsdemo_info.py

Co-authored-by: Poruri Sai Rahul <[email protected]>

* rename ModelView as DemoModelView

* fix scatter_inspector2

* remove todo list

Co-authored-by: Poruri Sai Rahul <[email protected]>

Co-authored-by: Poruri Sai Rahul <[email protected]>
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.

Contribute examples to the etsdemo application
2 participants