simphony / simphony-mayavi

The mayavi adapters to the simphony framework
BSD 2-Clause "Simplified" License
0 stars 1 forks source link

Remove EngineManagerStandalone #173

Closed kitchoi closed 8 years ago

kitchoi commented 8 years ago

Originally EngineManagerStandalone was meant to replicate the functions of the EngineManagerStandaloneGUI or EngineManagerMayavi2 without the use of a GUI. However, since changes in the __init__ of CUDSSource, EngineSource made creating a Source more user-friendly plus the fact that the EngineManager* has not grown as big as was anticipated, there is little gain now to keep EngineManagerStandalone around. Therefore proposing getting rid of it.

Also, "fix" 1/3 of the issue in #172 (by removing the problem....hmm)

@itziakos, @stefanoborini

kitchoi commented 8 years ago

Not yet done with cleaning the doc for this yet. Want to know what you guys think.

stefanoborini commented 8 years ago

:+1: for me.

kitchoi commented 8 years ago

@itziakos, what do you think? if this is good I will go on to clean the related documentation before removing the WIP tag.

itziakos commented 8 years ago

Yes, please move forward with the changes

stefanoborini commented 8 years ago

What's the status of this now?

kitchoi commented 8 years ago

@stefanoborini I believe this only needs updating the documentation.

stefanoborini commented 8 years ago

Should I get rid of the examples, or convert them to a different strategy? (it's unclear to me how to do non-gui, considering that now you seem to need a panel to achieve animations, as far as I understand from the tests)

kitchoi commented 8 years ago

@stefanoborini The batch scripting approach (already in the doc) with some modification using mlab.animate should be enough to achieve animation. In my opinion, it is fine to just get rid of the current example and rely on mayavi's doc. Providing an example using mlab.animate would be being-kind-to-new-users :)

codecov-io commented 8 years ago

Current coverage is 91.63% (diff: 100%)

Merging #173 into master will decrease coverage by 0.15%

@@             master       #173   diff @@
==========================================
  Files            47         45     -2   
  Lines          2193       2164    -29   
  Methods           0          0          
  Messages          0          0          
  Branches        320        319     -1   
==========================================
- Hits           2013       1983    -30   
  Misses          145        145          
- Partials         35         36     +1   

Powered by Codecov. Last update 059493c...af57712

stefanoborini commented 8 years ago

@kitchoi please review. There is apparently a change in behavior with most up-to-date packages, so I had to fix the versions.

kitchoi commented 8 years ago

Thanks @stefanoborini Is it possible to skip a particular version of a particular package (e.g. flake8 != 3.0.0) instead of fixing the versions for all packages?

stefanoborini commented 8 years ago

Question is: is it good practice to leave the version unspecified? I understood it wasn't from previous experiences.

kitchoi commented 8 years ago

There has been a similar discussion on https://github.com/simphony/simphony-framework/issues/15 My own preference is to skip known-problematic versions, this makes dependency resolution a lot easier.

stefanoborini commented 8 years ago

The test failure is due to this commit in mayavi

f3fd83b57137b3ac09309ffbb4d3c529352265b2 is the first bad commit
commit f3fd83b57137b3ac09309ffbb4d3c529352265b2
Author: Prabhu Ramachandran <prabhu@aero.iitb.ac.in>
Date:   Sun Jul 3 00:38:06 2016 -0400

    Support recording movie with animate decorator.

    If one has a scene with the movie_maker.record set to True, a movie will
    be recorded when the animation is run.

:040000 040000 1755a2f37a626855627403309e850f13527ecb2d ca0ac36cda6e985e61f581e4fce01c6add49197b M  mayavi
:040000 040000 7a9b3c7c6367ddce6dd3bc01b8bd7e8dac88d655 92b0c79f5cac9591530d59f0f6838b7a7addbfcd M  tvtk
stefanoborini commented 8 years ago

@kitchoi any comment on this?

stefanoborini commented 8 years ago

After discussion with @itziakos we agreed to do the following for the current red: