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

additional notebook. #19

Merged
merged 2 commits into from
Nov 15, 2017
Merged

additional notebook. #19

merged 2 commits into from
Nov 15, 2017

Conversation

miguelcleon
Copy link

I could also do this second demo if there is time.

I tried adding the ui_utils into the code folder but I'm not able to import. It is commented out here:
https://github.com/BiG-CZ/wshp2017_tutorial_content/compare/master...miguelcleon:master?expand=1#diff-2f207230c34a63ce64f450c0f31b1228R21

@emiliom
Copy link
Member

emiliom commented Nov 15, 2017

Thanks for the offer, @miguelcleon. The part I like from your notebook is the insertion of a new variable per se, the last two cells (but mostly cell 10) and the explanatory text before cell 10. The rest is largely duplicative of your other example notebook and doesn't add anything new in my opinion -- and that's a bunch of code to not demo any significantly new aspect 😜.

I think that kernel of new stuff may be better suited as an add-on to another notebook, such as Example 3 or something else that @horsburgh may be putting together to demo odm2api. So, I'll leave the PR unmerged for @horsburgh to think about and borrow liberally.

As for using your ui_utils code, how about using sys.path.insert() to add the code folder path? See ODM2 Examples 1-3 for examples of the path extraction and concatenation used to point to the data folder (which is at the same hierarchy level as code), as an example; that generic code will make the path reference work on both JupyterHub and a local install. But, two more comments about this module:

  • If you're going to use it in a notebook that will go on the repo, please rename it to something less generic in this broader context (eg, odm2admin_ui_utils, miguel_ui_utils, etc)
  • You import bokeh. bokeh is not on our conda env file. It would need to be added, and Don would have to deploy it on JupyterHub (that's a manual step). He could do that, but I'd first like to have some confidence that it will actually be used.

Thanks!

@horsburgh
Copy link
Member

I wasn’t planning on developing new content for the ODM2 Python API discussion.

@miguelcleon
Copy link
Author

Ok, I can successfully import the now named mcl_ui_utils, I removed bokeh. I removed the listing of CVVariableName. @emiliom I think this addresses your comments.

@emiliom
Copy link
Member

emiliom commented Nov 15, 2017

@miguelcleon I'm not clear about the full extent of changes in this new commit. It'll be easier if you tell me in person in a bit.

@emiliom emiliom merged commit 696f20c into BiG-CZ:master Nov 15, 2017
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.

3 participants