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 error messages #59

Merged
merged 13 commits into from
Nov 20, 2023
Merged

Conversation

maltelueken
Copy link
Collaborator

@maltelueken maltelueken commented Nov 8, 2023

Fixes the error message for Hayes models that are not implemented to appear correctly (previously did not appear at all).

Fixes the error message when a linear local test type is chosen but a factor variable is in the model.

Removes the footnote about model saturation from the model summary table.

Changes the footnote about model converge to appear as a symbol for the corresponding models in the model summary table.

Adds a disclaimer about causal effects to parameter estimates output.

Copy link
Member

@Kucharssim Kucharssim left a comment

Choose a reason for hiding this comment

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

The code changes look ok to me, but there are some general issues:

There still some kind of state issue; In this file (change extension from .jasp to .zip): process-bug.zip

I get Error in object@Options: no applicable method for '@' applied to an object of class 'NULL', right when I created the second model

If I refresh the analysis, I get a different error. Then if I remove the first model, the second model runs fine.

Otherwise, the tests fail quite catastrophically on Ubuntu. I assume you did not have the opportunity to test it there?

I think we could merge this, but tt would be good to make sure that the module is not broken for Linux users for the release...

R/classicProcess.R Outdated Show resolved Hide resolved
@maltelueken
Copy link
Collaborator Author

maltelueken commented Nov 17, 2023

Thanks for the review! I fixed the state issue (it was actually a container issue) and the issue with the summary table if one model is not estimated yet (invalid). I also used the jaspBase functions where possible.

Turns out that containers for new models were not correctly added to jaspResults, so their objects could not be accessed.This is fixed now and I moved all state dependencies to the container (because they are the same for all states in the container).

I also found out that the tests for models 82-92 were not correct (missing input), which I fixed.

@maltelueken
Copy link
Collaborator Author

maltelueken commented Nov 17, 2023

Regardint the failing tests on Linux: This is happening since JASP moved to R 4.3.1, so I think it might be a seed issue. I will try to run the tests on my WSL to see if they also fail there.

EDIT: There are two issues I think:

  • There are very small differences between result and reference for most failing tests. I don't know why they occur.
  • For a few tests, there are large differences in the standard errors and CIs. That is probably beceause the models are almost not identifiable due to correlated variables (contcor1, contcor2, and debCollin1). Thus, tiny differences in the estimated vcov matrix can lead to very large differences in standard errors.

Both issues are due to lavaan I think, as I could reproduce the differences in lavaan without the module.

@maltelueken
Copy link
Collaborator Author

I fixed a bug that caused the large differences between Linux and the other systems for some failed tests. The bug was that covariances where estimated between mediator variables that also had a causal path between them.
The other tests still fail due to differences at the 5th decimal. This will never be visible to the users, so I think it is ok to leave this for now.

@maltelueken maltelueken merged commit 28e5396 into jasp-stats:master Nov 20, 2023
3 of 4 checks passed
@maltelueken maltelueken deleted the fix-error-messages branch November 20, 2023 12:54
@maltelueken maltelueken mentioned this pull request Nov 20, 2023
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