Closed jwcodee closed 1 year ago
Thanks for all this work Johnathan and Parham!
A couple of additional comments in addition to the couple in the review:
- Have you updated the docs?
- Could you add tests for the python wrappers? I think it would be good for us to add those upfront when adding new btllib code moving forward
I haven't updated the docs. The readme said to run ninja docs before the release itself.
Sure I can add python tests.
It's ok to generate docs here since we're releasing straight after merging
It's ok to generate docs here since we're releasing straight after merging
I agree - also think it's best to keep the master branch code in sync with the docs
Updated docs generated with conda version of doxygen (1.9.6 (c4c52d9716f313ded2deb5e7cdbc02bbcf389ad3*)). Seems like some docs were generated with a previous version.
Thanks for the changes @jwcodee. I don't want to be blocking this from being merged while I'm off, so I'll approve for now. Feel free to merge whenever everything's working fine.
Based on Vlad's suggest, I used constexpr inline
instead of static const
but this feature requires c++17
. When c++17
is enabled, clang-tidy flags new errors. Simply changing the instances unique_ptr
to make_unique
generates new errors that are not easily fixed so I opted to use nolint.
When c++17 is enabled, clang-tidy flags new errors.
I also encountered these errors when I wanted to integrate argparse
, which requires C++17, into our recipes. Created #90 to track this in a future PR.
Murathan has resolved the python wrapper issue. The problem seems to be that the string data got cleared by some interaction. Murathan made a new private member to store a copy and that alleviates the issue. I am testing if it will increase ram substantially in the RAM test.
If this does increase the RAM usage, we should consider switching our std::string&
s to std::string_view
s (https://github.com/bcgsc/ntHash/issues/45).
If this does increase the RAM usage, we should consider switching our
std::string&
s tostd::string_view
s (bcgsc/ntHash#45).
That worked. I am now using std::string_view
in place of const std::string&
and reverted Murathan's proposed suggestion.
This PR contains aaHash and associated changes to integrate aaHash into the btllib environment.
Non-exhaustive list of changes
seq_reader
andseq_writer
to read and write amino acid sequencesnte
function to accelerate generate multiple hashesI intend to make a release v1.6.0 PR as soon as this PR is approved. As we are trying to submit aaHash as soon as possible, there are some outstanding items to be implemented
TBD in 1.6.X: