-
Notifications
You must be signed in to change notification settings - Fork 1
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
PyOpenSci REVIEW - 59 some more minor changes #60
Conversation
Codecov ReportBase: 97.50% // Head: 97.50% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
=======================================
Coverage 97.50% 97.50%
=======================================
Files 5 5
Lines 160 160
=======================================
Hits 156 156
Misses 4 4 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
meta_values = [ | ||
[ | ||
str(v).replace("nan", "") | ||
for k, v in pgap.get_meta_info_for_HMM(hmm).items() | ||
if k != "#ncbi_accession" | ||
] | ||
for hmm in hmm_group.split("|") # row.hmm.split("|") | ||
for hmm in row.hmm.split("|") |
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.
🔥
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.
If you feel the need to go all the way into types hints with mypy, you will have to change a few things.
However, this is IMO a pretty great code base at the moment 🚀
Addressing these issues from reviewer @Batalex in PyOpenSci
I have not profiled the code to see if there is a performance bottleneck there, but I would use itertuples instead of iterrows in addHMMmetaInfoToHits. itertuples is much faster if you need to iterate over data frames one row at a time
This was changed in this PR
Here getting rid of
getattr
My personal recommendation is to define the text encoding whenever a text file descriptor is opened. Even if this is not feasible at the moment, this is a tiny step toward Windows compatibility, and it removes a little bit of uncertainty.
The MG1655.gb file is stored in the tests folder, which is not included in the package. Therefore, when the package is installed using conda, it is not downloaded.
There are a few discrepancies in types, some of them could be fixed by converting argparse.ArgumentParser instances to CommandArgs before using them
Here using typing's union to indicate flexible type