Repository navigation
Implement thresholding in graph reconstructors - #93
Merged
Merged
Conversation
Also clean up imports to utilities somewhat.
Collaborator
Author
|
This is a big PR, so we should definitely talk things through and carefully examine what we're doing before merging this. |
This commit adds the ability to pass `binary=True` to `**kwargs` to have thresholding functions return the binary matrix instead of the thresholded, but still continuous, weight matrix.
tlarock
approved these changes
Feb 13, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #38. #57 introduced utilities for thresholding matrices in various ways, but they were largely not used. This incorporates them into the existing graph reconstructors where feasible.
There are a few reconstructors that do not threshold. For example, the minimum spanning tree does not threshold (because it is a minimum spanning tree). The one edge case here (which I did not touch) is convergent cross-mapping, which thresholds a matrix of p-values (i.e., a significance test). This could, in principle, be covered by the
thresholdutility function, but I don't think it's entirely appropriate to do so. (Giving the method the ability to set the significance level to get a fixed average degree seems to me more problematic than dropping low correlations to get a fixed average degree, but opinions may vary here.)As a side effect of implementing this feature, the
fit()method has been changed across all implementations (including the base reconstructor) to take arbitrary named keyword arguments through**kwargs. This allows us to pass threshold parameters freely, even to non-thresholding methods (where the parameters are ignored), which makes using this feature much easier in practice (see the change tonetrdExploreras an illustration of why).As a side effect, this PR also adds tests for the thresholding functions and cleans up a number of the imports to
netrd.utilities.Finally, I'll note that some of the language used in the different implementations differs; e.g.,
matrixversusweights. Instead of addressing that in code review of this PR I would suggest raising an issue and PR-ing that separately.