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

Ensure OpenMx is the package associated with MxRAMModel objects #16

Closed
tbates opened this issue Feb 10, 2017 · 18 comments
Closed

Ensure OpenMx is the package associated with MxRAMModel objects #16

tbates opened this issue Feb 10, 2017 · 18 comments

Comments

@tbates
Copy link
Member

tbates commented Feb 10, 2017

summary() bug or deep mystery of the universe

http://openmx.ssri.psu.edu/thread/4199
Is the package (OpenMx) associated with MxRAMModel objects not being stored as part of the class definition?

@tbates
Copy link
Member Author

tbates commented Mar 28, 2017

FYI, teaching a class, about 5/50 people had this error last week. It seemed to have something to do with package order. Went away with reinstall package, restart R.

@jpritikin
Copy link
Member

No idea how to reproduce this or whether it's even an OpenMx problem. Closing until we receive a better bug report.

@RMKirkpatrick
Copy link
Contributor

I wonder if this issue is worth revisiting in light of thread 4311?

@mcneale
Copy link
Contributor

mcneale commented Nov 11, 2017 via email

@RMKirkpatrick
Copy link
Contributor

But upgrading didn't resolve the issue. What did was krzysiek re-running his MxModels anew, instead of loading them from a saved workspace.

@RMKirkpatrick
Copy link
Contributor

Should we revisit this issue? It's still tripping users up.

@mcneale
Copy link
Contributor

mcneale commented Jan 29, 2018 via email

@tbates
Copy link
Member Author

tbates commented Jan 29, 2018 via email

@mcneale
Copy link
Contributor

mcneale commented Jan 29, 2018 via email

@RMKirkpatrick
Copy link
Contributor

N.B. that we DO warn the user if s/he is running an MxModel that was created with a version of OpenMx different from the one currently loaded. See line 72 in R/MxModel.R and line 21 in R/MxRun.R .

@RMKirkpatrick
Copy link
Contributor

I'm reopening this issue, because I think I finally have a fix for it. Today, I read the R Internals manual, concerning S4 classes (plus another document to which the manual links). Based on what it said, I began to suspect that this issue is caused by MxRAMModels never being initialize()d as instances of that class. Instead, they are only ever initialize()d as instances of the base class, "MxModel". Then, what happens to turn them into MxRAMModels is that a function called by mxModel() uses class()<- to change their class to "MxRAMModel" per the value of mxModel() argument type. That does not suffice to give them the "package" attribute, set to "OpenMx"; that's because S4 classes are defined within the context of a package, whereas the end user's R script can use class()<- to change an object's class arbitrarily (even to an undefined class!).

As a proof of concept, I attach summary.MxRAMModel.diff.txt, which is a git diff relative to 7ebcc10 . If I rebuild OpenMx after making the changes reflected in the diff, I can run loadtest--181129.R, save R's workspace, restart R, and load the saved workspace, and OpenMx is automatically loaded into R's workspace with it. That is the current behavior for objects of class "MxModel", but that is new behavior for objects of class "MxRAMModel". I'm running R in RStudio, FYI.

For reference, this is the forums thread about this issue: https://openmx.ssri.psu.edu/thread/4199 .

@RMKirkpatrick RMKirkpatrick reopened this Apr 9, 2019
@RMKirkpatrick
Copy link
Contributor

The changes reflected in that diff are just proof-of-concept. The actual implementation of the bug-fix will have to be less crude. We should probably discuss it at a meeting. And we should implement analogous changes for MxLISRELModels, too.

@jpritikin
Copy link
Member

Wow, Rob, accolades to your tenacity on solving this one!

@RMKirkpatrick
Copy link
Contributor

@jpritikin Thanks!

@RMKirkpatrick
Copy link
Contributor

RMKirkpatrick commented Apr 9, 2019

Oh, ba80a3f , for Pete's sake! Can it be that the bug-fix that has eluded us all this time IS A SINGLE LINE OF CODE?!

Fingers crossed that travis and the nightly run both pass.

Edit: well, travis failed, but that wasn't my fault.

@RMKirkpatrick
Copy link
Contributor

If class() is allowed to change the class identifier of an object to an arbitrary string, then it's allowed to change the class identifier of an object into a string with an arbitrary attribute: https://openmx.ssri.psu.edu/comment/8122#comment-8122

@RMKirkpatrick
Copy link
Contributor

Well, it looks like the fix for this issue was much simpler than I thought. Contrary to my initial suspicion, it's not actually necessary for objects of class "MxRAMModel" to be initialize()d as such, and unlike the diff I posted upthread, implementing the repair required only one line of code.

The nightly run passed all tests. Could someone please independently verify that it works? You should be able to build R from the head of the master branch, run loadtest--181129.R, let it save R's workspace, restart R, load the saved workspace, and see OpenMx get automatically loaded into R's workspace with it.

Once someone confirms that it works, this issue can be closed.

@RMKirkpatrick
Copy link
Contributor

I'm going to close this issue. We can reopen it if it turns out my fix doesn't work.

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

No branches or pull requests

4 participants