Fix get_engine options agreement #303

Merged
merged 2 commits into from Mar 9, 2016

Conversation

Projects
None yet
3 participants
@kitchoi
Member

kitchoi commented Feb 29, 2016

  • If someone switches offscreen on, gets an OffScreenEngine created and registered, then switches offscreen off again, get_engine would still return OffScreenEngine if `backend=="auto". This doesn't make sense (or should it?). So here I implemented a fix for this special case.
  • Test for get_engine is made more robust by using assertIs instead of assertIsInstance as NullEngine, EnvisageEngine, OffScreenEngine are all subclass of Engine (so only L40 of the master test_engine_manager.py is dubious). But this is a crucial distinction for the new test added in this PR

@kitchoi kitchoi changed the title from Fix get engine options agreement to Fix get_engine options agreement Feb 29, 2016

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 2, 2016

Member

I am a bit unsure about this. The reason is that "auto" is supposed to mean: pick a reasonable default. The trouble is that "reasonable" is quite ambiguous. Maybe the solution would be to add more unambiguous backends -- say an "offscreen" backend or a "ui" backend? Are you thinking that "auto" is just another name for "ui"? Thoughts?

Member

prabhuramachandran commented Mar 2, 2016

I am a bit unsure about this. The reason is that "auto" is supposed to mean: pick a reasonable default. The trouble is that "reasonable" is quite ambiguous. Maybe the solution would be to add more unambiguous backends -- say an "offscreen" backend or a "ui" backend? Are you thinking that "auto" is just another name for "ui"? Thoughts?

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Mar 2, 2016

Member

Indeed "reasonable" is not well defined. So it would be helpful to check some case studies. For example:

from mayavi import mlab
mlab.options.offscreen = True
mlab.test_contour3d()
mlab.savefig("saved.png")

mlab.options.offscreen = False
mlab.figure()  ### Here

should the reasonable engine on the last line be an Engine (i.e. on screen) and not an OffScreenEngine?

Member

kitchoi commented Mar 2, 2016

Indeed "reasonable" is not well defined. So it would be helpful to check some case studies. For example:

from mayavi import mlab
mlab.options.offscreen = True
mlab.test_contour3d()
mlab.savefig("saved.png")

mlab.options.offscreen = False
mlab.figure()  ### Here

should the reasonable engine on the last line be an Engine (i.e. on screen) and not an OffScreenEngine?

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 9, 2016

Member

Hmm, I think if offscreen is false, it should pop-up a view but then again, since there is already an off screen engine ready, technically it could use the same off screen engine.

OK, so if offscreen is not set, one should not get an offscreen engine. So in your case, one should get a non-offscreen engine depending on the backend. Is this what happens with your PR?

Member

prabhuramachandran commented Mar 9, 2016

Hmm, I think if offscreen is false, it should pop-up a view but then again, since there is already an off screen engine ready, technically it could use the same off screen engine.

OK, so if offscreen is not set, one should not get an offscreen engine. So in your case, one should get a non-offscreen engine depending on the backend. Is this what happens with your PR?

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Mar 9, 2016

Member

OK, so if offscreen is not set, one should not get an offscreen engine. So in your case, one should get a non-offscreen engine depending on the backend. Is this what happens with your PR?

Indeed this PR is aiming at this.

Member

kitchoi commented Mar 9, 2016

OK, so if offscreen is not set, one should not get an offscreen engine. So in your case, one should get a non-offscreen engine depending on the backend. Is this what happens with your PR?

Indeed this PR is aiming at this.

prabhuramachandran added a commit that referenced this pull request Mar 9, 2016

@prabhuramachandran prabhuramachandran merged commit 3b40750 into master Mar 9, 2016

5 checks passed

codecov/project 43.94% (target 40.00%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@prabhuramachandran prabhuramachandran deleted the fix-get-engine-options-agreement branch Mar 9, 2016

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