archivesunleashed / auk-notebooks

Jupyter notebooks to assist in creating additional analysis and visualizations of Archives Unleashed Cloud derivatives.
https://cloud.archivesunleashed.org
Other
11 stars 5 forks source link

Include functions for testing existence of derivatives. #10 #37

Closed greebie closed 5 years ago

greebie commented 5 years ago

This is a response to #10 that includes the following:

To test:

I've been changing the filenames from 4867-fullurls.txt to NOT4867-fullurls.txt and then running the windows to see how it works. Then you just remove the NOT to restore the original.

ruebot commented 5 years ago

If there is a function that tests the existence of a derivative file, why aren't we displaying that error for the graphs? Just returning two giant empty graphs is going to confuse the user, especially when we do it silently. How about we just return the error that is setout in check_main?

greebie commented 5 years ago

I was trying to avoid wrapping the code in an if-else, because it seemed messy. I agree this is not the right approach. Let me try something else, like throw an error or something.

greebie commented 5 years ago

Upcoming commit uses a defined function and run_function() if TEXT else logging.error ('etc.') syntax instead. Might be a little more confusing for people not familiar with function definitions, but less messy than wrapping the commands in an if-else.

greebie commented 5 years ago

hmm. Cannot replicate this with my repo. Was this the "example" notebook or the plain and do you get the same problem with both?

greebie commented 5 years ago

Also, are you on 3.7 -- fstrings may not work if Python < 3.7.

ianmilligan1 commented 5 years ago

I can update my Python to see if that matters, but since this worked before, it is possible to use something other than fstrings?

ianmilligan1 commented 5 years ago

Yeah, there's a few other DH packages etc. that are still pegged to Python 3.5. Unless its absolutely mission critical, it'd be good to keep it working w/ earlier versions.

greebie commented 5 years ago

I can replace it, but the specs do say we require Python 3.7 or up. I can see the fstrings feature being useful for a project like this. It would be nice to keep it if we can.

Fstrings means we can create any variable and add the value to a string using {variable}. That means it can be very useful for madlibs-type work.

Also, it does not look like the error is fstrings-related. It's just that I cannot tell without being able to replicate.

greebie commented 5 years ago

I propose we look at getting this repo to work in 3.7, remove the extra notebook and then do another commit to revert to 3.5 support, even if that means moving to "string".format or something. I can just imagine watching dominos fall trying to accommodate 3.5 in this PR.

greebie commented 5 years ago

Had a change of mind. Switched to .format strings. If we want to support 3.5 we'll need to change the Docker settings.

greebie commented 5 years ago

Closing in favor of #40 .