Skip to content

/auk-notebooks

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

Create user variables for easier output. #30

Merged
merged 13 commits into from Mar 7, 2019

Conversation

Reviewers

@ruebot ruebot

@ianmilligan1 ianmilligan1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@greebie @ruebot @ianmilligan1
@greebie
Copy link
Collaborator

greebie commented Mar 6, 2019

I pulled out the user variables to make changing the visualization both a little more flexible and easier to see.

Some conventions:

  • I used namespaces DOMAIN_, TEXT_, OVERALL_ and NETWORK_
  • In some cases I used an override DOMAIN_ if (DOMAIN_) else CONFIG_
  • user vars should follow the CAPS_PLUS_UNDERSCORE format.

In some cases, additional descriptions were added to show what's going on a bit better.
@greebie

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

greebie commented Mar 6, 2019

Made the changes in docker and they did not carry over. Going to have to redo it. :(
@ruebot

This comment has been minimized.

Sign in to view
Copy link
Member

ruebot commented Mar 6, 2019

@greebie Docker is READ ONLY unless you setup your filesystem mount so that it isn't. Best to do the edits locally.
@greebie

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

greebie commented Mar 6, 2019 •
edited

Thanks, yeah - I got lazy because I didn't have nltk.stopwords installed. I managed to port my work over from the read only version, fortunately.

I thought I could get away with trying to download the file, but no go.
@greebie
Fix errors in notebook.
Loading status checks…
3212488

@greebie greebie referenced this pull request Mar 6, 2019

Open

Not all collections will produce all derivatives #10

@greebie

greebie reviewed Mar 6, 2019
View changes

auk-notebook.ipynb Outdated
@@ -752,7 +855,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.7.1"
"version": "3.7.0"

This comment has been minimized.

Sign in to view
Copy link
@greebie

greebie Mar 6, 2019

Author Collaborator

This is going to keep changing as different people have their hands on it. Not sure what to do here.

This comment has been minimized.

Sign in to view
Copy link
@ruebot

ruebot Mar 6, 2019

Member

It's fine.
@ianmilligan1

ianmilligan1 requested changes Mar 6, 2019
View changes

Copy link
Member

ianmilligan1 left a comment

Looks great, and runs well. My comments here are all variations on a theme, just providing more context around the variables.

I also caught one variable error.
Show resolved Hide resolved auk-notebook.ipynb
Show resolved Hide resolved auk-notebook.ipynb Outdated
Show resolved Hide resolved auk-notebook.ipynb
Show resolved Hide resolved auk-notebook.ipynb
Show resolved Hide resolved auk-notebook.ipynb Outdated
Show resolved Hide resolved auk-notebook.ipynb Outdated
Show resolved Hide resolved auk-notebook.ipynb Outdated
Show resolved Hide resolved auk-notebook.ipynb

@ruebot ruebot referenced this pull request Mar 7, 2019

Closed

Fix malformed JSON for collection ID #31

@ianmilligan1

This comment has been minimized.

Sign in to view
Copy link
Member

ianmilligan1 commented Mar 7, 2019

Let me know when you're ready for me to review again @greebie! Looking forward to checking it out.
@greebie

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

greebie commented Mar 7, 2019

One more tiny fix that I missed and we are good to go.
@ruebot

ruebot requested changes Mar 7, 2019
View changes

Copy link
Member

ruebot left a comment •
edited

This variable is the most important, and the only variable you need to change to see a complete set of visualizations for your Archives Unleashed Cloud Derivatives.

Lower case derivatives, and might as well link to this https://cloud.archivesunleashed.org/derivatives

  • Inconsistent use of full stops and capitalization on comments. Please put full stops on all comments, and captialize the first word. We don't want to undo #22.

  • Fix the comment with OUTPUT_FILENAME again. Inconsistent variables in comment and code.

  • Be consistent with whitespace below a comment. Sometimes there is a blank line, sometimes there isn't one. Choose one or the other.

  • Cell 32 needs a leading comment.

  • Cell 32 needs a leading comment, and white space at the beginning of the line.

  • Cell 34 needs a leading comment.

  • Cell 36 needs a leading comment.

  • Cell 39 needs a leading comment.

  • Cell 43 needs a leading comment.

  • Cell 45 needs a leading comment.

  • Cell 47 needs a leading comment.

  • Cell 51 needs a leading comment.

  • Cell 52 needs a leading comment.

  • Cell 53 needs a leading comment.

  • NetworkX citation is missing a full stop at the end.
@ianmilligan1

ianmilligan1 requested changes Mar 7, 2019
View changes

Copy link
Member

ianmilligan1 left a comment

Looking good on my end.

I missed this earlier, but I think we should also pop out the user-defined variable for cell 38. SELECT_DOMAIN or something like that? Maybe markdown above cell 38 explaining the variable, then a single-line cell with a variable to set?
@greebie

This comment has been minimized.

Sign in to view
Copy link
Collaborator Author

greebie commented Mar 7, 2019 •
edited

I did a weird thing with OUTPUT_FILENAME. I set it so that filename is TEXT_OUTPUT_FILENAME if that is set, otherwise it uses OUTPUT_FILENAME. Next commit will fix periods etc.
@greebie
Fix spacing and periods.
Loading status checks…
9f9d2c0
@ruebot

This comment has been minimized.

Sign in to view
Copy link
Member

ruebot commented Mar 7, 2019

@greebie can you update the branch too?

greebie added some commits Mar 7, 2019

@greebie
Link to derivatives.
Loading status checks…
5f73a6b
@greebie
Merge branch 'master' into issue-28
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1e80ebb
@ruebot

ruebot requested changes Mar 7, 2019
View changes

Copy link
Member

ruebot left a comment

Still have inconsistent capitalization and missing full stops on a number of comments.
@ruebot

ruebot requested changes Mar 7, 2019
View changes

Copy link
Member

ruebot left a comment

  • Cell 3: Still missing full stops on a few of the comments.
  • Cell 4: Capitalize the first work on the first five in-line comments.
  • Cell 23: Still missing full stops on in-line comments, and the first word needs to be capitalized in a few.
  • Cell 24: Capitalize the first work on the last three in-line comments.
  • Cell 25: First in-line comment missing a full-stop.
  • Cell 27: First in-line comment missing a full-stop.
  • NetworkX citation is still missing a full stop at the end.
@ruebot

ruebot approved these changes Mar 7, 2019
View changes

greebie added some commits Mar 7, 2019

@greebie
Merge branch 'master' into issue-28
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e65d35c
@greebie
Add text-by-domain extraction variable.
Loading status checks…
d28ed45
@ianmilligan1

ianmilligan1 approved these changes Mar 7, 2019
View changes

@ianmilligan1 ianmilligan1 merged commit 3a873cc into master Mar 7, 2019
1 check passed

1 check passed

ci/dockercloud Your tests passed in Docker Cloud
Details

@ianmilligan1 ianmilligan1 deleted the issue-28 branch Mar 7, 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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.