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

Display R warnings #104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Kucharssim
Copy link
Member

@Kucharssim Kucharssim commented Nov 22, 2022

fixes https://github.com/jasp-stats/INTERNAL-jasp/issues/2138

It's kind of a hacky quick fix but it works nicely so why not?

Two questions regarding the implementation:

  1. The warnings are also caught and displayed if we run the analysis from jaspTools (which also "cleans up" the output in our unit tests). The question is whether we want to do that since perhaps it's valuable to see all warnings in the testing output?
  2. The only warnings that are displayed are those that occurred during the most recent analysis run. So if a particular code produced a warning but it's not run anymore because its output is saved in a state, it will dissapear from that list. Do we want to have a persistent (within an analysis) and ever growing list of warnings instead?

@vandenman
Copy link
Contributor

I'm not sure if this is such a good idea. R can throw many many warnings. If the warnings matter, the developer should catch them and show them to the user in an appropriate manner (e.g., as a footnote, or jaspHtml). If the warnings do not matter, then we probably don't want to show them to users. In that case, showing the warnings would only cause unnecessary confusion.

For example, consider the warnings in the linked issue.

1: In cor.smooth(r) : Matrix was not positive definite, smoothing was done
2: In fa.stats(r = r, f = f, phi = phi, n.obs = n.obs, np.obs = np.obs,  :
  the model inverse times the r matrix is singular, replaced with Identity matrix which means fits are wrong

which actions should a user take when they are confronted with this? Most likely they will either (1) do nothing or (2) post an issue on GitHub/ forum.

Another downside of this approach is that the warnings are not translateable, which they would be translateable if the developer catches them properly.

I do think it is a good idea to enable this when developer mode is on.

@Kucharssim
Copy link
Member Author

I'm not sure if this is such a good idea.

Well, it was your idea: jasp-stats/jaspFactor#125 (comment)

I agree that ideally the code should appropriately handle messages/warnings/errors, but the issue was that sometimes it's not that easy if the packages we rely on do not do that correctly either.

I do think it is a good idea to enable this when developer mode is on.

Yeah we could do that instead, sure. I am not even opposed to scratching this idea altogether, but I was of the impression the consensus of the linked discussion was to implement this :)

@vandenman
Copy link
Contributor

Well, it was your idea: jasp-stats/jaspFactor#125 (comment)

hahaha well looks like my past and present self are in disagreement then 😅.

We could also merge this as an "experiment" and then see whether people (e.g., EJ but intentionally not tagged) complain about it and then adjust the behavior based on that.

@Kucharssim
Copy link
Member Author

I actually don't hate the idea of enabling it only with a developer mode on (or making an additional option under preferences). Seems like a good option to eat the cake and keep it too?

@Kucharssim
Copy link
Member Author

I do think it is a good idea to enable this when developer mode is on.

Done!

@Kucharssim
Copy link
Member Author

Do you want to do anything about this, @vandenman?

@JorisGoosen JorisGoosen force-pushed the master branch 2 times, most recently from 8b639a0 to 7bd7444 Compare March 20, 2024 14:06
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.

None yet

2 participants