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

Fixing tests #129

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Fixing tests #129

merged 2 commits into from
Oct 8, 2024

Conversation

lassedochreden
Copy link
Collaborator

relates to arm64 build errors on BioC

Fixed the error for countInteractions() - seems to be related to small floating point errors

Did not fix the error in test_integration_interactions.R - did not find a quick fix - potentially related to different NA handling (see screenshot below) - would be great, if @nilseling could have a look.

Will update NEWS and DESCRIPTION upon approval.

Screen Shot 2024-10-07 at 17 48 07

@nilseling
Copy link
Collaborator

Thanks for looking into it! Could you figure out which p value is correct by comparing the count against the permutations? imcRtools let's you set the return_samples parameter for this.

@nilseling
Copy link
Collaborator

Ok, in this case I'm 95% sure that the old implementation of the function is generating a wrong p value due to floating point issues. Would be good to double check though

@lassedochreden
Copy link
Collaborator Author

lassedochreden commented Oct 8, 2024

Also now fixed test_integration_interactions.R - Was related to machine precision issues, which we did not consider when manually calculating the p-values (despite doing so in the function implementation).

Will merge, if you agree.

@lassedochreden lassedochreden merged commit 3270d3a into devel Oct 8, 2024
1 of 2 checks passed
@lassedochreden lassedochreden deleted the test_fix branch October 8, 2024 13:40
@lassedochreden
Copy link
Collaborator Author

@SchulzDan - Please push to BioC, when you can.

@nilseling
Copy link
Collaborator

I noticed that there is this max(ct_obs)==0 call in utils.R when calculating the p values. And since a count of 0 is now not detected as 0 anymore I wondered if this also needs to be replaced.

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