Closed anton-bushuiev closed 1 year ago
Thanks for this @anton-bushuiev. Good spot!
LGTM! Would you be able to add a couple tests?
Yeah, I'll add them tomorrow.
Hi, @a-r-j ! I've added the tests. I had to fix a new bug introduced by the distance matrix filtering (see this) 😄.
Kudos, SonarCloud Quality Gate passed!
Base: 40.27% // Head: 47.86% // Increases project coverage by +7.59%
:tada:
Coverage data is based on head (
fd1b36b
) compared to base (8123f42
). Patch coverage: 52.20% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Thanks for the contribution @anton-bushuiev!
Reference Issues/PRs
No related issues/PRs
What does this implement/fix? Explain your changes
This pull request fixes a bug in
add_k_nn_edges
. Currenly,kneighbors_graph(X=dist_mat, ...)
leads to wrong results which may seem correct. According to scikit-learn docs, it does not supportX
to be a distance matrix.Secondly, this PR adds
filter_distmat
to generalise self-loops filtering. It adds the functionality to filter inter- or intra- connections between nodes of a single or multiple chains. It may be useful to incorporate it into other functions to add edges. By running the following code you will get the visualized graphs.What testing did you do to verify the changes in this PR?
Pull Request Checklist
./CHANGELOG.md
file (if applicable)./graphein/tests/*
directories (if applicable)./notebooks/
(if applicable)python -m py.test tests/
and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g.,python -m py.test tests/protein/test_graphs.py
)black .
andisort .