Closed YojanaGadiya closed 1 year ago
Not sure a note like this is super valuable, here's how I'd go about this:
chembl_downloader.query()
(https://github.com/cthoyt/chembl-downloader#run-a-query-and-get-a-pandas-dataframe)tabulate
and export the GitHub format that shows for each version of chembl if the query was possible, if not, where it failed, and if so, the resultsI see. Okay. It is going to take some time, but I will try and send a PR soon.
@cthoyt for the 1st point with regard to statistics, do you have anything specific in mind or should I just print the total no.of chemicals and then add it to the tabulate?
Total number of chemicals sounds good
@cthoyt check out the commit. It writes to the README directly.. but I am sure you would want to error to be more cleaner.. any pointers for me?
Please run tox -e lint,flake8
and clean up the code then I can take another look.
In fffd13c, I added the following functionality:
chembl_downloader.history
Please don't worry about automatically re-writing the README, this is not so important
@YojanaGadiya all you need to do now is re-run the script
@cthoyt I think the PR is ready to be merged. Please check if this is according to what you expected.
@cthoyt the table is now updated. Please check if this is how you planned.
Merging #5 (388a6c9) into main (ceda5e6) will decrease coverage by
1.62%
. The diff coverage is3.57%
.
@@ Coverage Diff @@
## main #5 +/- ##
==========================================
- Coverage 22.95% 21.32% -1.63%
==========================================
Files 6 7 +1
Lines 305 333 +28
Branches 65 68 +3
==========================================
+ Hits 70 71 +1
- Misses 232 259 +27
Partials 3 3
Impacted Files | Coverage Δ | |
---|---|---|
src/chembl_downloader/downloader_checker.py | 0.00% <0.00%> (ø) |
|
src/chembl_downloader/queries.py | 60.86% <100.00%> (+1.77%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Does anything need to be done with respect to coverage?
No don't worry about coverage but the rest are required
Please check the latest commits. Is this similar to what you expected?
@YojanaGadiya better, but there's no need for a duplicate yes/no column at this point since there is either a number or a dash that says the same information.
If you can fix that and pass tox, this is ready to merge.
@cthoyt I fixed the README and flake8 on my end. Please can you check/approve the workflow to confirm. Thank you.
Apologies for spamming. Hopefully the last commit to the PR. Please approve @cthoyt
Note that before merging, I deduplicated a lot of code. And combine the new table with the existing one. Thanks @YojanaGadiya.
Closes #4
This PR adds total compound counts via the SQLite dump, which implicitly documents for which versions SQLite is available.