Motion view dev #82

Merged
merged 159 commits into from Aug 12, 2014

Conversation

Projects
None yet
3 participants
@tarzzz
Contributor

tarzzz commented Aug 1, 2014

This branch contains some major changes in the pydy.viz module. Which I am listing here. I am opening this Pull request to have a final review before it is merged. @moorepants @chrisdembia @jcrist @srjoglekar246

P.S: I am working on JS-Testing in a separate branch #74 I plan to get this one merged once that is complete, and merged into motion-view-dev.

New Features:

  • A new, better UI.
  • Integration with IPython notebooks. For rendering the visualizer inside the IPython notebooks, simply call scene.display_ipython from a notebook. (Works only on latest master of IPython).
  • Now rendered objects can be modified from the UI itself(using edit objects button in UI), and new JSON will be generated, which can be viewed by clicking view model button in UI.
  • Added a new property "material" to the rendered shapes (Shape object).
  • Allows rerunning the simulations(after changing the simulation parameters) from the visualizer itself. This feature is partially implemented, but will be fully functional once System class comes in.
  • Javascript backend completely modified. Now it is object oriented using Prototype.js.

Instructions for building this branch with latest IPython, and testing the new features:

  1. Cloning and installing latest IPython:

    $ git clone https://github.com/ipython/ipython ipython-dev
    $ cd ipython-dev
    $ git submodule update
    $ pip install -e ".[notebook]"

  2. get this branch from git repo:

    $ cd to pydy-repo
    $ git checkout --track origin/motion-view-dev

  3. Add it to PYTHONPATH.

    $ export PYTHONPATH="/your/pydy/repo/:$PYTHONPATH"

  4. Now you can check out the double_pendulum example.

    from visualize import *
    scene.display_ipython()

Please feel free to contact me, if there are any difficulties!!

Conda method for testing:

$ conda create -n new-pydy-viz numpy scipy matplotlib theano cython sympy
$ source activate new-pydy-viz
(new-pydy-viz)$ conda remove ipython  # conda automatically installs ipython into envs
(new-pydy-viz)$ git clone https://github.com/ipython/ipython
(new-pydy-viz)$ cd ipython
(new-pydy-viz)$ git submodule update
(new-pydy-viz)$ pip install -e ".[notebook]"
(new-pydy-viz)$ cd ..
(new-pydy-viz)$ git clone https://github.com/pydy/pydy
(new-pydy-viz)$ cd pydy 
(new-pydy-viz)$ git checkout --track origin/motion-view-dev
(new-pydy-viz)$ cd examples/double_pendulum
(new-pydy-viz)$ ipython notebook

TODOS(based on suggestions in this conversation):

I have copied original suggestions to maintain the context.

  • Update sphinx docs.
  • Users should not need to set PYTHONPATH.
  • Commit an ipython notebook for the double_pendulum example.
  • The model isn't loaded by default, you have to press "Load Simulation". It's nice that the file path is there, but I think it should load the scene when you call "display_ipython".
  • There are two boxes for editing the simulation parameters. There should be only one.
  • Also this needs to handle 0 to 100-1000 parameters. The simple models on a have a few parameters, but most models have hundreds if not thousands. We probably don't want to display them all to the screen, but maybe let the user scroll through them somehow.
  • The time indicator jumps from 1 to 2 decimal places. Make sure to use a formatter so this always has the same # decimals. Also you should set the decimals based on the min and max time. Some simulations may run at 100000 hz, some may run at 10 hz. We will need a way to speed up and slow down the simulation. It should default to running at real time.
  • I liked the time bar indicator. It would be cool to have that and it be drag-able.
  • The loop checkbox is missing. I like that feature.
  • It is not clear what the "Show Model" button is supposed to do. I though it would make the scene load at first but that didn't happen.
  • I like the object editor, that works nice! how the little menu pops up after selecting the object. The color doesn't seem to change the color. It would be cool if you can rotate and translate the objects relative to their default position here too. I think MG View allows that.
  • The rerun simulation button doesn't seem to work.

Some final TODOs before merge(as discussed with @chrisdembia ), with priorities:

  • 5 cube
  • 1 decimal places
  • 3 remove mesh from dropdown
  • 4 remove double default from dropdown
  • 2 default dimensions
  • 8 play pause
  • 6 sharp UI
  • 7 blink
  • Integrate with System class
  • Merge JS-Tests
  • Repeated pressing of pause and/or stop have issues.
  • When you rerun simulation, do you have to then press "load simulation"
  • Wasn't the load sim button a file selection dialog before?
  • If you change model parameters, rerun, and then load the sim it seems like the order of the model parameters changes. If you reload the notebook cell, then the parameter order changed. There is something fishy going on there and you are changing the wrong parameters.
  • The stop button disappears when the simulation is running.
  • Everytime you load a sim in the directory it asks to overwrite the static directory. This shouldn't work like that if you have data files in there. We don't want to erase data files. They need to be preserved so you can load them for future use. We only want to erase the other generated files. What is the purpose of the static dir? Just for data files or also for the html/css/js stuff?
  • Dragging the time slider while the animation is playing doesn't work. Either you should be allowed to do that or it will move you to a new place in the animation and keeping playing from there.
  • If you Play the sim, then pause it, then drag the slider, then play it again doesn't start playing fro where you moved the slider. It plays from where you moved the slider from.
  • Changing the color doesn't always work for the shape.
  • If the shape is blue the blinking blue isn't that useful to know what you are editing.
  • The stop button dissappears when the sim is playing but reruning the notebook cell doesn't fix it. I would think that reruning the notebook cell would rebuild the widget and everything would be as new.
  • It would be cool to render the parameters with mathjax so they should their mathematical representation in the notebook.
  • If you slide the slider when the animation is not playing, then you press play the animation starts from the initial time instead of where the slider is at.
  • If you select "edit objects" and select a shape then change the color and press apply, the color of the shape does not change. I think this is related to the "default" material only.
  • I think it would be better if the JSON dialog box was below the scene visualization. Right now, it pushes the scene down the screen, which is odd.
  • It'd be nice to have xyz labels on the global axes, and ideally the axes/labels could be toggled with a checkbox.
  • The user should be able to modify the initial conditions and time array via the GUI so that it is easier to rerun simulations.
  • The user should be able to specify custom filenames in the GUI and save a simulation after they fiddled with the simulation parameters via the GUI.
  • Scene.display() no longer seems to open up the scene into a new browser tab, the user has to do it manually.
@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Jun 2, 2014

Contributor

Can you add a little more detail here saying that two files will be written, <outfile_prefix>_scene_desc.json, etc.?

Contributor

chrisdembia commented on pydy/viz/scene.py in 2f90858 Jun 2, 2014

Can you add a little more detail here saying that two files will be written, <outfile_prefix>_scene_desc.json, etc.?

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Jun 2, 2014

Contributor

This sentence is a little unclear to me ("A prefix...files").

Contributor

chrisdembia commented on pydy/viz/scene.py in 2f90858 Jun 2, 2014

This sentence is a little unclear to me ("A prefix...files").

pydy/viz/scene.py
+ self.generate_visualization_json(self.dynamic_variables,
+ self.constant_variables, self.dynamic_values,
+ self.constant_values,fps=self.fps,
+ outfile_prefix=self.outfile_prefix)

This comment has been minimized.

@tarzzz

tarzzz Aug 10, 2014

Contributor

@moorepants Here is where I use those saved values!

@tarzzz

tarzzz Aug 10, 2014

Contributor

@moorepants Here is where I use those saved values!

@tarzzz

This comment has been minimized.

Show comment
Hide comment
@tarzzz

tarzzz Aug 10, 2014

Contributor

Steps to integrate System in Scene(for personal reference as well!):

  • Add time_array as an attribute to Scene.
  • Create a new generate_visualization_json_system(to pull specifieds, and dynamic_values etc.)
  • Inside Rerun Simulation:
  • Call sys.integrate
  • Call generate_visualization_json.
    Done!!
  • Modify Tests
  • Modify Examples.
Contributor

tarzzz commented Aug 10, 2014

Steps to integrate System in Scene(for personal reference as well!):

  • Add time_array as an attribute to Scene.
  • Create a new generate_visualization_json_system(to pull specifieds, and dynamic_values etc.)
  • Inside Rerun Simulation:
  • Call sys.integrate
  • Call generate_visualization_json.
    Done!!
  • Modify Tests
  • Modify Examples.
@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

That plan for System sounds right to me...keep chugging along!

Contributor

chrisdembia commented Aug 10, 2014

That plan for System sounds right to me...keep chugging along!

@tarzzz

This comment has been minimized.

Show comment
Hide comment
@tarzzz

tarzzz Aug 10, 2014

Contributor

@chrisdembia what if I create a separate function, like generate_visualization_json_from_system(system) for now? When we have both Langrange and Kanes implemented in system class, we can get rid of the existing method, and replace it, so it looks like:
generate_visualization_json(system).

For the time being a user uses KanesMethod, he can use either of them, but if he uses Langranges, he is bound to use the one which requires four args(2 specifieds, 2 states) instead of the one which requires one arg(system).

Editing the existing generate_visualization_json will make it messier, and since we are going to get rid of the existing method(which takes four arguments, two for specifieds, two for states) anyways. How does that sound? @moorepants

Contributor

tarzzz commented Aug 10, 2014

@chrisdembia what if I create a separate function, like generate_visualization_json_from_system(system) for now? When we have both Langrange and Kanes implemented in system class, we can get rid of the existing method, and replace it, so it looks like:
generate_visualization_json(system).

For the time being a user uses KanesMethod, he can use either of them, but if he uses Langranges, he is bound to use the one which requires four args(2 specifieds, 2 states) instead of the one which requires one arg(system).

Editing the existing generate_visualization_json will make it messier, and since we are going to get rid of the existing method(which takes four arguments, two for specifieds, two for states) anyways. How does that sound? @moorepants

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

good idea!

On Sun, Aug 10, 2014 at 11:01 AM, Tarun Gaba notifications@github.com
wrote:

@chrisdembia https://github.com/chrisdembia what if I create a separate
function, like generate_visualization_json_from_system(system) for now?
When we have both Langrange and Kanes implemented in system class, we can
get rid of the existing method, and replace it, so it looks like:
generate_visualization_json(system).

For the time being a user uses KanesMethod, he can use either of them, but
if he uses Langranges, he is bound to use the one which requires four
args(2 specifieds, 2 states) instead of the one which requires one
arg(system).

Editing the existing generate_visualization_json will make it messier,
and since we are going to get rid of the existing method(which takes four
arguments, two for specifieds, two for states) anyways. How does that
sound? @moorepants https://github.com/moorepants


Reply to this email directly or view it on GitHub
#82 (comment).

Contributor

chrisdembia commented Aug 10, 2014

good idea!

On Sun, Aug 10, 2014 at 11:01 AM, Tarun Gaba notifications@github.com
wrote:

@chrisdembia https://github.com/chrisdembia what if I create a separate
function, like generate_visualization_json_from_system(system) for now?
When we have both Langrange and Kanes implemented in system class, we can
get rid of the existing method, and replace it, so it looks like:
generate_visualization_json(system).

For the time being a user uses KanesMethod, he can use either of them, but
if he uses Langranges, he is bound to use the one which requires four
args(2 specifieds, 2 states) instead of the one which requires one
arg(system).

Editing the existing generate_visualization_json will make it messier,
and since we are going to get rid of the existing method(which takes four
arguments, two for specifieds, two for states) anyways. How does that
sound? @moorepants https://github.com/moorepants


Reply to this email directly or view it on GitHub
#82 (comment).

pydy/viz/scene.py
+ self.generate_visualization_json(system.states,
+ system.constants_symbols,
+ system.integrate(),
+ system.constants, **kwargs)

This comment has been minimized.

@tarzzz

tarzzz Aug 10, 2014

Contributor

@chrisdembia I think, I am giving the wrong arguments here(It is giving an error with modified double_pendulum example). Can you help me figuring them out?

I need in the args in following order:
dynamicsymbols, constant_symbols, dynamics_values_numerical, constant_values_numerical

what attributes in system correspond to the above mentioned?

@tarzzz

tarzzz Aug 10, 2014

Contributor

@chrisdembia I think, I am giving the wrong arguments here(It is giving an error with modified double_pendulum example). Can you help me figuring them out?

I need in the args in following order:
dynamicsymbols, constant_symbols, dynamics_values_numerical, constant_values_numerical

what attributes in system correspond to the above mentioned?

This comment has been minimized.

@tarzzz

tarzzz Aug 10, 2014

Contributor

nevermind, fixed it. I should be using system.constants.values() as the fourth arg.

@tarzzz

tarzzz Aug 10, 2014

Contributor

nevermind, fixed it. I should be using system.constants.values() as the fourth arg.

@tarzzz

This comment has been minimized.

Show comment
Hide comment
@tarzzz

tarzzz Aug 10, 2014

Contributor

Well, this adds System to Scene. Some brief desc. of new implementations:

If KanesMethod is being used, a user can use either generate_visualization_json or generate_visualization_json_system. If he prefers the latter one, and provides system as arg, we can re-integrate the system, hence the re-run simulations actually re-integrates the system. I have implemented this in double_pendulum example

Some tests, sphinx docs, and it is good to go in. (well need to merge js-testing here too!)

Contributor

tarzzz commented Aug 10, 2014

Well, this adds System to Scene. Some brief desc. of new implementations:

If KanesMethod is being used, a user can use either generate_visualization_json or generate_visualization_json_system. If he prefers the latter one, and provides system as arg, we can re-integrate the system, hence the re-run simulations actually re-integrates the system. I have implemented this in double_pendulum example

Some tests, sphinx docs, and it is good to go in. (well need to merge js-testing here too!)

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

"It has to" -> "It must" is usually more proper.

Contributor

chrisdembia commented on pydy/system.py in e5ca34c Aug 10, 2014

"It has to" -> "It must" is usually more proper.

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

array_like, not dict, right?

Contributor

chrisdembia commented on pydy/system.py in e5ca34c Aug 10, 2014

array_like, not dict, right?

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

"User" -> "the user"; "the integration module" -> System.ode_solver.

Contributor

chrisdembia commented on pydy/system.py in e5ca34c Aug 10, 2014

"User" -> "the user"; "the integration module" -> System.ode_solver.

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

Thanks for fixing the tests. This addition looks very good! I think we can make some simplifications. Instead of checking len(times) == 0, just check times is None, and don't set times to [] in the constructor. It is conceivable I think that someone uses an ode_solver that accepts a scalar final time, and assumes the initial time is 0.

Contributor

chrisdembia commented on e5ca34c Aug 10, 2014

Thanks for fixing the tests. This addition looks very good! I think we can make some simplifications. Instead of checking len(times) == 0, just check times is None, and don't set times to [] in the constructor. It is conceivable I think that someone uses an ode_solver that accepts a scalar final time, and assumes the initial time is 0.

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 10, 2014

Contributor

Thanks for working on this so quickly Tarun! I will check it out later today.

Contributor

chrisdembia commented Aug 10, 2014

Thanks for working on this so quickly Tarun! I will check it out later today.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Aug 11, 2014

Member

times should have a default value, maybe the default of np.linspace(0, 10). So the default does not need a kwarg.

times should have a default value, maybe the default of np.linspace(0, 10). So the default does not need a kwarg.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Aug 11, 2014

Member

I don't think this is needed. The ode_solver should accept an "array like" and the errors will be raised there.

I don't think this is needed. The ode_solver should accept an "array like" and the errors will be raised there.

+ if self.system is not None:
+ #update system constants
+ self.system.constants = dict(zip(self.system.constants, self.constant_values))
+ self.generate_visualization_json_system(self.system)

This comment has been minimized.

@tarzzz

tarzzz Aug 11, 2014

Contributor

@chrisdembia If system is provided, this part of code is responsible for re-run

@tarzzz

tarzzz Aug 11, 2014

Contributor

@chrisdembia If system is provided, this part of code is responsible for re-run

@chrisdembia

This comment has been minimized.

Show comment
Hide comment
@chrisdembia

chrisdembia Aug 11, 2014

Contributor

@moorepants, @tarzzz seems to have fixed most of the bugs we brought up, and re-running a simulation works now. Do you want to give this another spin?

Contributor

chrisdembia commented Aug 11, 2014

@moorepants, @tarzzz seems to have fixed most of the bugs we brought up, and re-running a simulation works now. Do you want to give this another spin?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Aug 11, 2014

Member

Great! I just tried it. It now seems highly functional. Even the rerun button seems to work correctly! Nice!

This seems ready to be merged to master now that it is functionaly and the major bugs are squashed.

@tarzzz Please make issues for all of the remaining non-critical bugs and feature requests.

Also the example problem is now printing an array to the screen. Not sure where that is. Please remove the print statement.

Member

moorepants commented Aug 11, 2014

Great! I just tried it. It now seems highly functional. Even the rerun button seems to work correctly! Nice!

This seems ready to be merged to master now that it is functionaly and the major bugs are squashed.

@tarzzz Please make issues for all of the remaining non-critical bugs and feature requests.

Also the example problem is now printing an array to the screen. Not sure where that is. Please remove the print statement.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Aug 11, 2014

Member

FYI, the show json button seems to be broken on the non-notebook view, i.e. scene.display(). It shows a garbled codemirror form.

Member

moorepants commented Aug 11, 2014

FYI, the show json button seems to be broken on the non-notebook view, i.e. scene.display(). It shows a garbled codemirror form.

chrisdembia added a commit that referenced this pull request Aug 12, 2014

@chrisdembia chrisdembia merged commit 0e15bb3 into master Aug 12, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@moorepants moorepants deleted the motion-view-dev branch May 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment