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

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

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@greebie
Copy link
Collaborator

greebie commented Mar 8, 2019

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

  • Functions for accessing derivatives for graphml, gexf, and filtered text. (The latter has a placeholder only TextIO object until I figure out a logical way to process the zipped data).
  • A function to test the existence of derivatives.
  • Bool Vars TEXT, DOMAINS & NETWORK for easy checking.
  • thing if TEXT else [] syntax to produce empty graphs instead of an error when no derivative exists.

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.

@greebie greebie requested review from ianmilligan1 and ruebot Mar 8, 2019

@ruebot

This comment has been minimized.

Copy link
Member

ruebot commented Mar 9, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 9, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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.

@ianmilligan1
Copy link
Member

ianmilligan1 left a comment

When running the "user configuration" cell, I received this error:

  File "<ipython-input-4-d23338b01f65>", line 242
    f"No file found to complete text analysis. Check that your text derivative filename is\nformatted `{auk_full_text}` and stored in the correct folder `{auk_fp}`"),
                                                                                                                                                                   ^
SyntaxError: invalid syntax

This was with the default data.

@greebie

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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

@ianmilligan1

This comment has been minimized.

Copy link
Member

ianmilligan1 commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

ianmilligan1 commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

greebie commented Mar 14, 2019

Closing in favor of #40 .

@greebie greebie closed this Mar 14, 2019

ruebot added a commit that referenced this pull request Mar 14, 2019

ianmilligan1 added a commit that referenced this pull request Mar 14, 2019

Add comment on try/catch, and remove explicit exception; #37. (#41)
* Add comment on try/catch, and remove explicit exception; #37.

@ruebot ruebot deleted the issue-10 branch Mar 14, 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.