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

Cache PDB downloads #260

Merged
merged 4 commits into from
Jul 4, 2023
Merged

Conversation

PlethoraChutney
Copy link
Contributor

Adds biotite caching, resolving #256. Tested by downloading a structure, turning off wifi, and re-loading it successfully. The cache folder is also created and populated with models.

I'm mostly working on this one to figure out how to add stuff to MolNodes, but it does work. It looks like there is a lot of duplicating default values --- I tried to copy your style but I'd be happy with feedback.

Copy link
Owner

@BradyAJohnston BradyAJohnston left a comment

Choose a reason for hiding this comment

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

I think you nailed it first try. This adds the feature exactly as I was hoping and from my testing works perfectly!

I don't know how you'd do it, but potentially adding some kind of test would be good. Testing is tricky with the addon as you have to run them through the headless scripts, but maybe a test that runs and checks that the file has been downloaded would be all we need.

Otherwise fabulous work and thanks for the contributions, it's basically ready to be merged with a couple of minor changes.

MolecularNodes/load.py Outdated Show resolved Hide resolved
MolecularNodes/load.py Outdated Show resolved Hide resolved
MolecularNodes/load.py Outdated Show resolved Hide resolved
MolecularNodes/ui.py Outdated Show resolved Hide resolved
MolecularNodes/ui.py Outdated Show resolved Hide resolved
@PlethoraChutney
Copy link
Contributor Author

I was already getting a bit concerned with having to keep track of the several definitions of the default cache site, so I created config.json. If there's a mechanism you'd prefer I'd be happy to change it though!

Otherwise, made all the changes you mentioned. The test I added only checks whether a file with the right name got downloaded --- I don't do any checks to make sure it's the right structure.

@PlethoraChutney
Copy link
Contributor Author

I guess, looking at it, the ui.py stuff doesn't need to know where the cache actually goes, so the default location only goes in one file. Sorta removes the need for config.json, I can remove if you want

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.01 🎉

Comparison is base (9eef1ea) 53.71% compared to head (4e8225d) 53.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
+ Coverage   53.71%   53.73%   +0.01%     
==========================================
  Files          15       15              
  Lines        1949     1952       +3     
==========================================
+ Hits         1047     1049       +2     
- Misses        902      903       +1     
Impacted Files Coverage Δ
MolecularNodes/ui.py 40.76% <0.00%> (-0.08%) ⬇️
MolecularNodes/load.py 92.27% <100.00%> (+0.07%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BradyAJohnston
Copy link
Owner

Otherwise, made all the changes you mentioned. The test I added only checks whether a file with the right name got downloaded --- I don't do any checks to make sure it's the right structure.

biotite is already quite comprehensively tested, so I think we only need to test that the file is actually cached and that should be all.

For the config.json, maybe we ditch it if it is only being used once, unless you can think of other things we should also be storing in there.

Copy link
Owner

@BradyAJohnston BradyAJohnston left a comment

Choose a reason for hiding this comment

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

Looking good! Minor comments but otherwise it's good to merge once those are done

MolecularNodes/load.py Outdated Show resolved Hide resolved
@PlethoraChutney
Copy link
Contributor Author

Agreed on both fronts. Even if I can think of a reason to use an external config it should be a different PR. And using None makes more sense for the functions.

@BradyAJohnston BradyAJohnston merged commit dac9d24 into BradyAJohnston:main Jul 4, 2023
4 of 5 checks passed
BradyAJohnston added a commit that referenced this pull request Sep 14, 2023
Support for caching PDB downloads
BradyAJohnston added a commit that referenced this pull request Sep 14, 2023
Support for caching PDB downloads
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.

None yet

2 participants