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

fix: bug in compute_transition_probabilities #159

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Conversation

ludvigla
Copy link
Contributor

@ludvigla ludvigla commented Jul 8, 2024

Description

The transition probabilities are invalid at k > 1 in compute_transition_probabilities which can influence the computation of local G scores. This PR contains a fix for this bug along with some additional minor fixes:

Changes

  • correctly row-normalize W_out matrix after matrix power computation
  • handle situation where use_weights=False to match R implementation
  • set normalize_counts=False as default to match R implementation

Fixes: exe-1867

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

PR checklist:

  • This comment contains a description of changes (with reason).
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • If a new tool or package is included, I have updated poetry.lock, and cited it properly
  • I have checked my code and documentation and corrected any misspellings
  • I have documented any significant changes to the code in CHANGELOG.md

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.78%. Comparing base (4404d87) to head (dbef2f4).

Files Patch % Lines
src/pixelator/graph/node_metrics.py 44.44% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #159      +/-   ##
==========================================
- Coverage   81.78%   81.78%   -0.01%     
==========================================
  Files         121      121              
  Lines        6787     6795       +8     
==========================================
+ Hits         5551     5557       +6     
- Misses       1236     1238       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ambarrio ambarrio left a comment

Choose a reason for hiding this comment

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

LGTM. Only comment is how did you discover this? Can you in this case include maybe a test to collect this knowledge that needs to always pass?

@ludvigla
Copy link
Contributor Author

ludvigla commented Jul 9, 2024

LGTM. Only comment is how did you discover this? Can you in this case include maybe a test to collect this knowledge that needs to always pass?

I discovered it when creating the local G tutorial for Python because the p-values didn't match with R. These things are quite difficult to detect which is why I think it's so important that more people play around with the method in their analysis.

I will add a test!

Copy link
Contributor

@johandahlberg johandahlberg left a comment

Choose a reason for hiding this comment

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

LGTM

@ludvigla ludvigla merged commit 3f36907 into dev Jul 9, 2024
14 of 15 checks passed
@ludvigla ludvigla deleted the exe-1867-bug-fix branch July 9, 2024 15:29
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.

3 participants