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

PhenoOptiLIME #21

Merged
merged 6 commits into from
Feb 12, 2025
Merged

PhenoOptiLIME #21

merged 6 commits into from
Feb 12, 2025

Conversation

DatSplit
Copy link
Contributor

@DatSplit DatSplit commented Feb 1, 2025

No description provided.

@ldingemans ldingemans self-assigned this Feb 3, 2025
@ldingemans ldingemans self-requested a review February 11, 2025 06:54
Copy link
Owner

@ldingemans ldingemans left a comment

Choose a reason for hiding this comment

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

Hey @DatSplit thanks so much for the PR, overall it looks great, just one comment about some comments that are left in the code and could probably be removed.

Also, it looks like the LIME tests now take very long on Windows and therefore are failing after 360 min, any idea what is going on there?

Copy link
Owner

Choose a reason for hiding this comment

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

This looks pretty good, but a lot of commented code still in there, maybe check if that can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, agreed! Removed superfluous comments.

Copy link
Contributor Author

@DatSplit DatSplit Feb 11, 2025

Choose a reason for hiding this comment

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

@ldingemans Noticed that as well, unfortunately I do not have any idea what it might be.
It seems to be an "Open" issue: actions/runner-images#7320.
Might as well remove it for now, as this issue seems to be GitHub specific?
However, it used to work before I assume, so maybe the above issue is not the cause.

@DatSplit
Copy link
Contributor Author

DatSplit commented Feb 11, 2025

@ldingemans Never mind my previous comment (does not seem to be an issue anymore). Windows failed after 91m, because test_negative_control_permutation in test_permutation_test.py had a p-value smaller than 0.05:
Could be an intermittent issue caused by the shuffling and sampling in test_negative_control_permutation?

----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\PhenoScore\PhenoScore\phenoscore\tests\test_permutation_test.py", line 53, in test_negative_control_permutation
    assert permutation_tester.p_value > 0.05
AssertionError```


@ldingemans ldingemans merged commit 46cd735 into ldingemans:develop Feb 12, 2025
4 checks passed
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