Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

updated demos, add CX WIP notebook #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

colleenXu
Copy link

Will refer to this pull request in the issues I raise soon. Updated demos with fixes and more explanation, added my notebook that includes drafts of explanations/documentation and demonstrations of issues.

@andrewsu
Copy link
Member

Lots of good issues raised here. My only comment is that I don't think we should add the CX_WIP notebook to the base branch. I think that notebook is super useful to illustrating the various issues and we should use it as a basis for improving code/notebooks/documentation. But adding that notebook itself into the base branch I think might create more confusion for potential users...

@colleenXu
Copy link
Author

colleenXu commented Aug 14, 2020

Perhaps I can keep CX_WIP in my fork or a side branch instead. It does demo the issues that I raise, so I'd like these issues to link to it wherever it ultimately goes.

What do you think of the changes to the PREDICT and EXPLAIN demo notebooks that are also in this request?

@andrewsu
Copy link
Member

Yes, agreed on keeping CX_WIP in a fork or branch. I haven't looked at the PREDICT/EXPLAIN demo notebook changes -- I'll defer to @kevinxin90 on those...

@kevinxin90
Copy link
Contributor

@colleenXu So pls first revert ur commits for your WIP notebook, meanwhile I will take a look at ur other notebooks. It will take some time, since it's very hard to understand the changes you make for a long jupyter notebook on github.

@ravila4
Copy link

ravila4 commented Aug 18, 2020

Hi, I recently found this tool that could be useful for reviewing Jupyter notebook diffs: https://github.com/jupyter/nbdime

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants