Skip to content
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

Kendall tau method added to mantel computation #1675

Merged
merged 18 commits into from
Feb 7, 2020

Conversation

AnnaTruzzi
Copy link
Contributor

@AnnaTruzzi AnnaTruzzi commented Nov 6, 2019

Please complete the following checklist:

  • I have read the guidelines in CONTRIBUTING.md.

  • I have documented all public-facing changes in CHANGELOG.md.

  • This pull request includes code, documentation, or other content derived from external source(s). If this is the case, ensure the external source's license is compatible with scikit-bio's license. Include the license in the licenses directory and add a comment in the code giving proper attribution. Ensure any other requirements set forth by the license and/or author are satisfied. It is your responsibility to disclose code, documentation, or other content derived from external source(s). If you have questions about whether something can be included in the project or how to give proper attribution, include those questions in your pull request and a reviewer will assist you.

  • This pull request does not include code, documentation, or other content derived from external source(s).

Note: REVIEWING.md may also be helpful to see some of the things code reviewers will be verifying when reviewing your pull request.

@AnnaTruzzi
Copy link
Contributor Author

In Khaligh-Razavi, S. M., & Kriegeskorte, N. (2014) it is suggested to prefer the Kendall's tau over Spearman's rho as correlation coefficient when comparing models in which the order of variables is important. This is for example the case in y work where I am comparing deep neural networks embeddings with brain neural patterns.
Within this pull request I have minimally modified the _mantel.py function to add the Kendall's tau as one of the possible correlation methods.

Reference:
Khaligh-Razavi, S. M., & Kriegeskorte, N. (2014). Deep supervised, but not unsupervised, models may explain IT cortical representation. PLoS computational biology, 10(11), e1003915.

@ElDeveloper
Copy link
Contributor

Thanks for the contribution @AnnaTruzzi, looks like there are conflicts with your branch. Also, would you mind adding a new test to exercise this choice?

@AnnaTruzzi
Copy link
Contributor Author

Hi, I resolved the conflict, sorry about that.
In the _mantel.py function I already added the Kendall tau test, are you referring to a different one?

@ElDeveloper
Copy link
Contributor

Thanks @AnnaTruzzi, really appreciate this. I was referring to adding a unit test in skbio/stats/distance/tests/test_mantel.py.

@AnnaTruzzi
Copy link
Contributor Author

Hi,
I am not sure about what unit test is required, I'm sorry. And I do not want to mess everything up so I'll ask you first.
Would that be enough to add 'kendalltau' method in line 62 ot the test_mantel.py script so that it becomes as follows:
self.methods = ('pearson', 'spearman','kendalltau').

@ElDeveloper
Copy link
Contributor

@AnnaTruzzi No need to worry about messing anything. Yes, you are correct we would need to add the method in that line as well as checking that the methods using that attribute also work. For example in line 99 and in line 183.

If you would like some pointers, here's a document where we give some suggestions on how to test your code.

I need to make sure that the the script test_mantel.py is up to date in order not to work on obsolete functions
@AnnaTruzzi
Copy link
Contributor Author

Added and ran in the debugger: the 39 tests ran were ok.
It did throw some warnings but not because of the kendalltau method. They were already there previously to my addition.
Since I am not sure whether they are relevant to you or for my pull request, I'll copy and paste them here:

  • scipy/stats/stats.py:3399: PearsonRConstantInputWarning: An input array is constant; the correlation coefficent is not defined.
    warnings.warn(PearsonRConstantInputWarning())

  • numpy/lib/function_base.py:2534: RuntimeWarning: invalid value encountered in true_divide
    c /= stddev[:, None]

  • scipy/stats/_distn_infrastructure.py:901: RuntimeWarning: invalid value encountered in greater
    return (a < x) & (x < b)

  • scipy/stats/_distn_infrastructure.py:901: RuntimeWarning: invalid value encountered in less
    return (a < x) & (x < b)

  • scipy/stats/_distn_infrastructure.py:1892: RuntimeWarning: invalid value encountered in less_equal
    cond2 = cond0 & (x <= _a)

Copy link
Contributor

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests, I've left a few comments in the PR.

skbio/stats/distance/tests/.vscode/settings.json Outdated Show resolved Hide resolved
skbio/stats/distance/tests/test_mantel.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ElDeveloper
Copy link
Contributor

@AnnaTruzzi, one more note: you'll need to resolve conflicts in the CHANGELOG.md file. Let me know if I can help.

AnnaTruzzi and others added 4 commits November 26, 2019 10:22
Edit as per PEP8 requirements

Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
Edit to follow changelog format

Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
@AnnaTruzzi
Copy link
Contributor Author

Hi, thank you for correcting the mistakes!
I applied all the requested changes and resolved the conflict in the changelog file.

Copy link
Contributor

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnnaTruzzi thanks for updating the PR. I just noticed the entries in the CHANGELOG.md file are in a section for an older release. Also, going through the test_mantel.py, I've noticed that there's a few other test cases that would be good to test. For example adding a check for 'kendalltau' in the method in line 88 test_statistic_same_across_alternatives_and_permutations, and the method in line 112 test_zero_permutations.

Apologies for not catching this before, thanks so much for your contribution.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
AnnaTruzzi and others added 7 commits November 27, 2019 09:35
Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
Co-Authored-By: Yoshiki Vázquez Baeza <yoshiki89@gmail.com>
@AnnaTruzzi
Copy link
Contributor Author

Hi, sorry for the later reply, I've been caught in some deadlines.
I fixed the CHANGELOG file and added the two tests you asked for
Let me know if I can do anything else!

@AnnaTruzzi
Copy link
Contributor Author

Hi @ElDeveloper, do I need to make other changes for this pull request?
Let me know, thank you!

@ElDeveloper
Copy link
Contributor

Hi @AnnaTruzzi, thanks for following up. The code changes look good, however I think the conflict resolution in the ChangeLog is still not correct. The diff still shows a few more bullet points that shouldn't be included in your PR. Can you fix that? The diff should only show * Added Kendall Tau as possible correlation .... Let me know if I can help somehow! 👍

@AnnaTruzzi
Copy link
Contributor Author

Hi! yes at this point I could use some help, I guess I'm missing some fundamental step. What I do to to solve the conflict is to open the changelog with the conflicting lines, and remove the two versions leaving only the right line and then I commit it. What am I doing wrong? Thank you!

@ElDeveloper
Copy link
Contributor

Nothing wrong per se, I think it might be just due to git's confusing nature 😆. I think you might be able to solve this if you edit the CHANGELOG.md file and remove lines 49, 48, 47, 46 and 15.

Screen Shot 2020-02-06 at 5 47 28 PM

@AnnaTruzzi
Copy link
Contributor Author

Thank you! Lines deleted

@ElDeveloper
Copy link
Contributor

Thanks so much!

@ElDeveloper ElDeveloper merged commit fab93d3 into scikit-bio:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants