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

Encode rowValue of a componentList when it contains variables #5723

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

boutinb
Copy link
Contributor

@boutinb boutinb commented Oct 29, 2024

The MixedModels analyses do not work because the random effects ComponentsList has for source the "Random effects grouping factors" Variables List. So the rowValue of the componentsList (the key of a row in the ComponentsList) is a variable (with possible interactions), and it must be also encoded with the right type.

For this the Term object should contain not only the type when it is a single variable, but also the types of each interaction when it contains several variables.

Another test has been added in the jaspTestModule encoding analysis.

NB: The Mixed Models still do not work after this fix, but @FBartos knows why. He needs first this fix to be able to fix completely the Mixed Models.

@boutinb boutinb requested a review from JorisGoosen October 29, 2024 14:54
Copy link
Contributor

@JorisGoosen JorisGoosen left a comment

Choose a reason for hiding this comment

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

I guess it works (a bit hard to see without a real testcase) and I wouldve like to see the Terms constructor expanded to have 'type' as as an argument.
But If I merge it now it ends up in the nightly...

columnTypeVec coTypes = combos.at(j).types();
types.insert(types.end(), coTypes.begin(), coTypes.end());

Term newTerm(termComponents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not expand the constructor?

@@ -81,7 +84,7 @@ class Term
QStringList _components;
QString _asQString;
bool _draggable = true;
columnType _type = columnType::unknown;
columnTypeVec _types = {columnType::unknown};
Copy link
Contributor

Choose a reason for hiding this comment

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

_types should be settable in the constructor (and can then have `columnType::unknown' as the default argument.

@JorisGoosen JorisGoosen merged commit f86cf17 into jasp-stats:development Oct 29, 2024
1 check passed
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