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

Mixed Models fixes #30

Merged
merged 38 commits into from
Aug 16, 2021
Merged

Mixed Models fixes #30

merged 38 commits into from
Aug 16, 2021

Conversation

FBartos
Copy link
Collaborator

@FBartos FBartos commented May 26, 2021

fixes: jasp-stats/jasp-test-release#1208
fixes: jasp-stats/jasp-test-release#1207
fixes: jasp-stats/jasp-test-release#1210
fixes: jasp-stats/jasp-test-release#1260
fixes: jasp-stats/jasp-test-release#1326
fixes: jasp-stats/jasp-test-release#1327
fixes: jasp-stats/jasp-test-release#1331
fixes: jasp-stats/jasp-test-release#1331
fixes: jasp-stats/jasp-test-release#1330
fixes: jasp-stats/jasp-test-release#1323

should fix: jasp-stats/jasp-test-release#1329 (the fix works when run from R interactively - the analysis produces the correct error, but it still fails as a development module, can't figure out why though :( )

@FBartos FBartos marked this pull request as ready for review June 2, 2021 05:29
@AlexanderLyNL
Copy link
Contributor

I can't install this as a developer module and test :(

Screenshot 2021-06-09 at 19 13 19

Copy link
Contributor

@AlexanderLyNL AlexanderLyNL left a comment

Choose a reason for hiding this comment

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

I'm don't understand why snake cased variable names sneaked into the code.

@FBartos
Copy link
Collaborator Author

FBartos commented Jun 12, 2021

hmm, I'm not getting the jaspGraphs error when using the last nightly

@AlexanderLyNL
Copy link
Contributor

Perhaps someone else should attempt to review this, since I'm unable to get this to run on my mac.

Copy link
Contributor

@AlexanderLyNL AlexanderLyNL left a comment

Choose a reason for hiding this comment

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

Basically, the standard style problems appear again. I'll have a better in-depth look later.

Copy link
Contributor

@vandenman vandenman left a comment

Choose a reason for hiding this comment

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

Some comments below, I didn't test (yet) if the issues are actually fixed.

Also, concerning the test figures, can you confirm that there are two figures less because these are skipped?

@vandenman
Copy link
Contributor

@FBartos whenever you're ready just re-request a review!

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.

Ufff, so I went through the linked issues, it seems that they are now fixed.

I went through the code relatively fast, so I did not check every line of code. But it seems generally ok now, including the style changes.

I have one small remark about renaming option values, when you figure that out I think this is ok to be merged.

@Kucharssim Kucharssim self-requested a review July 23, 2021 13:47
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.

Typically, it's a good idea to test whether your fixes actually fix the issue. It's also good idea to read documentation properly (especially if you do something for the first time).

Below, suggested fixes to your fixes 😉

@FBartos
Copy link
Collaborator Author

FBartos commented Jul 26, 2021

Typically, it's a good idea to test whether your fixes actually fix the issue. It's also good idea to read documentation properly (especially if you do something for the first time).

Below, suggested fixes to your fixes 😉

Thanks, these are all fair points.

To be honest, I did not know how to test it.

@FBartos
Copy link
Collaborator Author

FBartos commented Jul 26, 2021

I tried to propagate the changes to the rest of the update, but the module just fails to be installed in JASP. Any idea why?
@Kucharssim

@FBartos
Copy link
Collaborator Author

FBartos commented Jul 26, 2021

It just crashes JASP hard

@Kucharssim Kucharssim self-requested a review July 26, 2021 10:25
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.

I did not know how to test it.

Yeah that's understandable. Basically, you need to create a .jasp file with a 0.14.1 JASP version and then load it up after installing your new version of the module. If the options that you changed show something like this
Screenshot 2021-07-26 at 12 18 47
Then the upgrade is not working.

but the module just fails to be installed in JASP. Any idea why?

No idea, it works for me now just fine... Maybe try Preferences -> Advanced -> Clean installed modules and packages, restart JASP and try install the module again. If that does not work, then I guess it's something for @JorisGoosen to look into.

Otherwise it looks good, I left one minor comment. I guess once we are sure the module is installing correctly, I am fine with merging it.

FBartos and others added 2 commits July 26, 2021 13:33
@FBartos
Copy link
Collaborator Author

FBartos commented Jul 26, 2021

I propagated the changes to the rest, tried removing the installed modules and packages but it didn't help:

image

@Kucharssim
Copy link
Member

I propagated the changes to the rest, tried removing the installed modules and packages but it didn't help:

It still does work for me, the upgrades are now also working properly. It could be a bug in a nightly, or something OS specific...? @JorisGoosen @vandenman any ideas?

If this is resolved, I think it's good to be merged.

@JorisGoosen
Copy link
Contributor

I propagated the changes to the rest, tried removing the installed modules and packages but it didn't help:

It still does work for me, the upgrades are now also working properly. It could be a bug in a nightly, or something OS specific...? @JorisGoosen @vandenman any ideas?

I've been looking into it, some other people also had issues. But for them it seems resolved now. Maybe also for @FBartos?

@FBartos
Copy link
Collaborator Author

FBartos commented Jul 29, 2021

Yes, it actually installed properly on the latest development nightly (I'm not touching that weird Czech stuff :D )

@Kucharssim Kucharssim self-requested a review July 29, 2021 09:57
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.

Unless @vandenman or @AlexanderLyNL object, I am willing to merge this monster.

@AlexanderLyNL
Copy link
Contributor

Unfortunately, I'm still unable to install and test this as a development module, not even with the latest nightly jasp-stats/jasp-desktop@a4e5065

@JorisGoosen
Copy link
Contributor

Unfortunately, I'm still unable to install and test this as a development module, not even with the latest nightly jasp-stats/jasp-desktop@a4e5065

https://github.com/jasp-stats/jasp-test-release/issues/1358 then?

@JorisGoosen
Copy link
Contributor

https://static.jasp-stats.org/Nightlies/MacOS/JASP-nightly-testMixedModelsFixes-e952e31081880b012e00522ddd66d5cb9d6978d6.dmg

A nightly which includes the translation fixes Frantisek just made, just for you @AlexanderLyNL

@FBartos
Copy link
Collaborator Author

FBartos commented Aug 10, 2021

Can you still install it as a development module? I wanted to fix: https://github.com/jasp-stats/jasp-test-release/issues/1209#issuecomment-895555271 but can't install it even without any additional changes (and I tried 3 different nightlies)

@AlexanderLyNL
Copy link
Contributor

I still can't install a development module with the new nightly and used Joris's special build to test.

@JorisGoosen
Copy link
Contributor

Should we maybemerge this and leave additional fixes for later?

This has been going on for long enough now I think. It just gets bigger and bigger

@vandenman
Copy link
Contributor

sure, let's merge this.

@vandenman vandenman merged commit 32f26c6 into jasp-stats:master Aug 16, 2021
@FBartos FBartos deleted the fixes branch August 21, 2021 09:34
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.

5 participants