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

Separate into 3 Notebooks - Issue 46 #51

Merged
merged 13 commits into from Apr 17, 2019

Conversation

Projects
None yet
3 participants
@greebie
Copy link
Collaborator

greebie commented Apr 11, 2019

The title says it all.

This adds a domain, text and network notebook instead of the previous "all-in-one" approach.

I added some additional analysis to domains because it was too short.

This will be in draft until all the edits are done. Final commit will remove the larger notebook.

greebie added some commits Apr 11, 2019

Small edits to domain notebook.
Tested results with second collection (results not shown here).
@ianmilligan1
Copy link
Member

ianmilligan1 left a comment

As requested, providing some initial feedback on the domains notebook. In-line mark-up is a bit ungainly for the changes I'm requested, so I will just provide them here.

This is looking good! On a fresh pull and kernel, the auk-notebook-domains file worked well and executed nicely.

I basically think we need to customize the opening section of the notebooks so that they're all bit different.

  • Let's make clear that this is a domain analysis notebook, maybe in the title or in the "Welcome" section;
  • Can we shrink the very long User Configuration section to only things that are necessary for the domains notebook specifically? As it is, it's a bit ungainly to have this in every notebook:

Screen Shot 2019-04-11 at 1 28 42 PM

Otherwise, I think the domains notebook is almost ready to go. Looking forward to reviewing the next ones (ping me in Slack or here when you want me to do a once over), although I think the feedback here is generalizable across the domains, text, and network ones.

@greebie greebie marked this pull request as ready for review Apr 15, 2019

@greebie

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Apr 15, 2019

I pulled this out of draft mode for review. I kept the current larger notebook inside the repo and will remove it as the last commit upon completion of the review requests.

@greebie greebie requested a review from ianmilligan1 Apr 15, 2019

@ianmilligan1
Copy link
Member

ianmilligan1 left a comment

Looks good - tested locally.

I might make the titles relate to the derivatives too, making what you put into the text after review even clearer. i.e. something like:

Screen Shot 2019-04-16 at 8 51 34 AM

But otherwise, looks good, and I think the next commit should remove the "all-together" notebook.

Improve network titles.
- Slight changes to introduction wording.
- Remove original notebook.
@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented on auk-notebook-domains.ipynb in ff00725 Apr 16, 2019

Please be consistent on "notebook" capitalization. It's different in each of the notebook files.

Copy notebooks to Dockerfile.
Standardize capitalization of "notebook."
@ianmilligan1
Copy link
Member

ianmilligan1 left a comment

Few minor stylistic changes and then its good to go on my end, thx @greebie

"\n",
"# Welcome to the Domains Analysis Notebook\n",
"\n",
"Welcome to the Archives Unleashed domains analysis Jupyter Notebook. This demonstration takes the domain derivatives from the Cloud and uses Python to analyze and produce information about your collection.\n",

This comment has been minimized.

Copy link
@ianmilligan1

ianmilligan1 Apr 16, 2019

Member

Let's capitalize Domains Analysis in Welcome to the Archives Unleashed domains analysis Jupyter Notebook. i.e. so that it reads Welcome to the Archives Unleashed Domains Analysis Jupyter Notebook.

"\n",
"Welcome to the Archives Unleashed domains analysis Jupyter Notebook. This demonstration takes the domain derivatives from the Cloud and uses Python to analyze and produce information about your collection.\n",
"\n",
"Please feel free to create an [issue](https://github.com/archivesunleashed/auk/issues) to let us know about any bugs you encountered or improvements you would like to see.\n",

This comment has been minimized.

Copy link
@ianmilligan1

ianmilligan1 Apr 16, 2019

Member

encountered -> encounter

"\n",
"# Welcome to the Text Analysis Notebook\n",
"\n",
"Welcome to the Archives Unleashed text analysis Jupyter Notebook. This demonstration takes the full text derivatives from the Cloud and uses Python to analyze the derivatives for your collection.\n",

This comment has been minimized.

Copy link
@ianmilligan1

ianmilligan1 Apr 16, 2019

Member

Same comment as above - let's capitalize "Text Analysis" here

"\n",
"Welcome to the Archives Unleashed text analysis Jupyter Notebook. This demonstration takes the full text derivatives from the Cloud and uses Python to analyze the derivatives for your collection.\n",
"\n",
"Please feel free to create an [issue](https://github.com/archivesunleashed/auk/issues) to let us know about any bugs you encountered or improvements you would like to see.\n",

This comment has been minimized.

Copy link
@ianmilligan1

ianmilligan1 Apr 16, 2019

Member

encountered -> encounter

This comment has been minimized.

Copy link
@ianmilligan1

ianmilligan1 Apr 16, 2019

Member

(please do this on the network file too)

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Apr 16, 2019

@greebie remove # Custom module included in folder. from the network, and text notebooks.

greebie added some commits Apr 16, 2019

@ruebot

ruebot approved these changes Apr 17, 2019

@ruebot ruebot merged commit e8b0fea into master Apr 17, 2019

1 check passed

ci/dockercloud Your tests passed in Docker Cloud
Details

@ruebot ruebot deleted the issue-46 branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.