-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Add API to download likelihood patchset archives #1046
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
+ Coverage 96.83% 96.87% +0.04%
==========================================
Files 60 62 +2
Lines 3507 3556 +49
Branches 504 510 +6
==========================================
+ Hits 3396 3445 +49
Misses 68 68
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@kratsg Given your comment on Issue #1045 RE: implementing Issue #947 what are your thoughts on this PR? It seems to me that having something like this PRs functionality of downloading a tarball and then extracting it into a directory is still in scope of |
cc53772
to
3b8fb74
Compare
3b8fb74
to
8258466
Compare
32e35be
to
f6f9ce8
Compare
840b567
to
2317bab
Compare
tests/test_scripts.py
Outdated
# hide | ||
CACHE_MODULE, sys.modules[module_name] = sys.modules[module_name], None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the only other thing to think about is if this should be made into a decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if pyhf contrib download
was hidden away if it didn't import correctly?
additionally, there should be a non-script/CLI test to check the python API itself.
Hm. Maybe I misunderstand what you mean, but we don't have a public Python API for this, we only have the CLI API that's exposed. |
It would generally make sense that we don't provide a CLI for anything that does not have a python API (for consistency) no? Maybe we need the python API first. |
I think I generally agree with that statement but if you're at the Python API do you really care about extracting files into directories rather than just reading them out from the in memory representation of the tarfile? I guess what you're thinking though is that if you wanted to script the download and unpacking of multiple likelihoods through the Python API you should be able to do that as well (correct?). |
Yes, or likely -- someone will want to parallelize and do a signal grid... But I'm realizing that this doesn't make sense given we only really ever have two files per grid anyway (bkgonly + patchset)... |
afc202c
to
90f9e80
Compare
The removal seems to not be effective in masking requests
This reverts commit 3af9b19. This is hard to test at the moment as don't have any of these, so hold off
fa735b0
to
b72e399
Compare
* Remove contrib download defaults for arguments ARCHIVE_URL OUTPUT_DIRECTORY * Amends PR #1046
* Migrate contrib download code from cli module to contrib module - Unify all of the contrib code in a single contrib module for easier factoring out and bookkeeping * Use DOI URL for all contrib download docstring examples * Amends PR #1046
Description
Resolves #1045
CLI API Example:
ReadTheDocs build:
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: