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

Add widgets for source & wavelength to WFIRST notebook interface #79

Merged

Conversation

josePhoenix
Copy link
Collaborator

  • Adds a source selection widget that depends on pysynphot
  • Adds a monochromatic calculation option

Joseph Long added 2 commits November 16, 2015 17:44
  - Adds a source selection widget that depends on pysynphot
  - Adds a monochromatic calculation option
@josePhoenix
Copy link
Collaborator Author

FYI this has only failed tests with old/stable POPPY (as expected).

@mperrin
Copy link
Owner

mperrin commented Nov 17, 2015

Not that it's particularly necessary, but do you know if it's possible to set up py.test to test jupyter notebooks and widgets? That sounds like something that would be a huge pain to setup and is probably not worth it, but perhaps the Jupyter or Travis folks have streamlined this sufficiently? (mostly I'm just thinking ahead for how we will set up test infrastructure to ensure things remain functional on e.g. the Jupyter server, not something we need to solve immediately.)

@josePhoenix
Copy link
Collaborator Author

Hmm, I thought about this a bit. The current architecture of display_notebook_interface (using a lot of closures as callbacks) wouldn't work well with a "mocking" based approach where we mock out the interfaces to the ipywidgets. I think trying to test interactions with the widgets (which involves setting up a headless browser and dealing with fiddly brittle javascript tests) is definitely beyond what we should attempt at this stage.

The best strategy might be to abstract all the stateful pieces out into a real GUI controller class and test the interfaces it exposes. Such a controller could then be used to back the Tk GUI as well (which has, alas, not fared well with the latest OS X updates... 😢)

@josePhoenix
Copy link
Collaborator Author

I just found a line missing in the notebook in this branch, stay tuned for one more commit...

@mperrin
Copy link
Owner

mperrin commented Nov 17, 2015

Yeah I'd noticed recently that now the Tk GUI will immediately segfault on my laptop, running either 2.7 or 3.4 from Macports. I had been chalking that up to some peculiarity or screwup from my Macports setup but are you saying this is a more global issue with Tk on recent Mac OS?

@josePhoenix
Copy link
Collaborator Author

Actually, I just meant it looks horrendous with the GUI style changes in El Capitan on my personal machine.

Buuuuttttt... I'm seeing a segfault now too on my work laptop. Yikes.

@josePhoenix
Copy link
Collaborator Author

Larger concerns about interfaces and testing aside, does this look good to you @mperrin ?

@mperrin
Copy link
Owner

mperrin commented Nov 18, 2015

I just downloaded and ran the notebook. Seems like it's all working nicely.

Minor aesthetic point: when displaying an optical system, the pupil scale shown is different for the first two planes. This is potentially slightly confusing to users and besides it's not an effective use of screen real estate to have the 2nd pupil zoomed out:

screen shot 2015-11-18 at 9 26 40 am

So that display could be tweaked, I think just by setting the pupil_diam attribute of the 2nd plane so it knows by default how large a region to display.

Second minor point: I think we should move the notebooks to a notebooks subdirectory, like is the case for poppy.

But neither of those should hold up this commit so I'll go ahead and merge it now.

mperrin added a commit that referenced this pull request Nov 18, 2015
…erface

Add widgets for source & wavelength to WFIRST notebook interface
@mperrin mperrin merged commit 8e72991 into mperrin:master Nov 18, 2015
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.

2 participants