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

Multisession modeling changes for AutoLFADS #50

Open
wants to merge 10 commits into
base: reorganize
Choose a base branch
from

Conversation

claytonwashington
Copy link

No description provided.

Copy link
Contributor

@cellistigs cellistigs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally things look really good! I had a few questions that I've left in the code, and if you could please restore tests that have been commented out, that would be much appreciated.

src/neurocaas_contrib/Interface_S3.py Outdated Show resolved Hide resolved
obj_keyname = obj.key
if (os.path.basename(obj_keyname) in os.listdir(localpath)) and (not force):
print("Data already exists at this location. Set force = true to overwrite")
return 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a way to 1) check if the folder we are writing to is empty? Or rather 2) to save time by refraining from re-downloading files?

Forgive me if I'm misunderstanding, but if 1) I think we'd want to check ahead of time, and not have things tied to individual file names. If 2)wouldn't we want to continue iterating other files even if one exists?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that this doesn't make sense. I'll modify to 2.

src/neurocaas_contrib/scripting.py Outdated Show resolved Hide resolved
return bucketname,username,data_dirname,config_dirname,s3_client,s3_resource


# def test_download(setup_simple_bucket,tmp_path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you restore this test (and test_upload?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, if you can restore tests here (including those that are skipped)

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

Successfully merging this pull request may close these issues.

2 participants