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: turns warnings into errors temporarily and internally #112

Merged
merged 2 commits into from
May 7, 2023

Conversation

mcanouil
Copy link
Owner

@mcanouil mcanouil commented May 7, 2023

To avoid complexity of the code, we use try() to catch errors and warnings by temporarily setting options(warn = 2), which turns warnings into errors.
Then, we set back the original value of options(warn).
This is not ideal, but it works.

Fixes #110

@mcanouil mcanouil self-assigned this May 7, 2023
@mcanouil mcanouil requested a review from burrowsk May 7, 2023 09:09
@burrowsk
Copy link
Collaborator

burrowsk commented May 7, 2023

That seems a sensible work around without doing anything too elaborate with the code.
However, I assume that if the first model call fails (for either warning or error) and the second call (with increased iterations) is initiated, that if this call produces warning messages then the call will complete? And the user will see the warning message?

@mcanouil
Copy link
Owner Author

mcanouil commented May 7, 2023

Yes, only the first call of the model.

Do we want absolutely no warnings at all?

@burrowsk
Copy link
Collaborator

burrowsk commented May 7, 2023

Thanks, I just wanted to make sure I understood it correctly.

My gut feeling is to allow warnings to run in order to assess the coefficients and allow a case by case decision on whether the models are good enough to use for that particular dataset. I think future users may prefer this too.

However, for the EGG projects it means trusting that the warning messages are picked up and reported back to the central team. Unless you have a strong opinion we should probably trust that warnings will be picked up and reported back.

@mcanouil
Copy link
Owner Author

mcanouil commented May 7, 2023

At least, in the code, I don't catch warnings.
I could by adding a bunch of code.

Here a proposal, let's keep the current workaround, open a new issue to improve how warnings and errors are captured to be worked on a bit later.

@burrowsk
Copy link
Collaborator

burrowsk commented May 7, 2023

Okay - that sounds like a good plan to me, I will approve the review now. Thanks very much!

@mcanouil
Copy link
Owner Author

mcanouil commented May 7, 2023

See #113 for improvement on how warnings are handled in the linear mixed model.

@mcanouil mcanouil merged commit 4538ff2 into main May 7, 2023
@mcanouil mcanouil deleted the fix/issue110 branch May 7, 2023 10:55
@mcanouil mcanouil added this to the v1.0.0 milestone Jan 27, 2024
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.

Increase of number of iterations for the EM algorithm niterEM
2 participants