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

DRAGON- Ambiguous exception thrown during calculation of p-values #334

Open
blawney opened this issue Dec 18, 2023 · 4 comments
Open

DRAGON- Ambiguous exception thrown during calculation of p-values #334

blawney opened this issue Dec 18, 2023 · 4 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@blawney
Copy link
Contributor

blawney commented Dec 18, 2023

The estimate_p_values_dragon function is raising an exception stating "f(a) and f(b) must have different signs"

@marouenbg
Copy link
Collaborator

marouenbg commented Dec 18, 2023

Hey @blawney, I am tagging @katehoffshutta for help, she may need a working example but I did get this issue before #298

@katehoffshutta
Copy link
Contributor

Thanks @marouenbg @blawney! @marouenbg no working example needed, I asked @blawney to raise this so we can correct the ambiguity of the exception.

As Marouen mentioned, the problem and fix(workaround) are described in detail in #298. The remaining issue is that we need to add an informative exception so that instead of seeing "f(a) and f(b) must have different signs", the user is given a description of the issue and suggested to use the MC p-value calculation rather than the parametric version.

I will get started on this!

@blawney
Copy link
Contributor Author

blawney commented Dec 19, 2023

Outside of the exception mentioned above, is the current recommended solution to catch the exception from the estimate_p_values_dragon and then subsequently fall back to estimate_p_values_mc with the same function arguments?

@katehoffshutta
Copy link
Contributor

Currently I am not planning to implement the default as fall back to estimate_p_values_mc at this point because it is somewhat time-consuming. We could provide the user an estimated run time for the MC calculation in the exception, I think, based on the size of their dataset.

That said, we are working on algorithmic improvements and parallelization and once those are in place, I think it would make sense to redirect as you describe above.

@violafanfani violafanfani added bug Something isn't working enhancement New feature or request labels Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants