-
Notifications
You must be signed in to change notification settings - Fork 133
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
Removes use of statsmodels.tsa.arima_model.ARMA #1896
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small changes. Other than that, it looks good. maybe @wangcj05 or @PaulTalbot-INL would like to take a second look.
I don't know if these will pass without updating the pinned version of statsmodels in RAVEN, in |
@j-bryan You can update that dependency in this PR if you'd like. Or is there already a PR updating |
@dylanjm @PaulTalbot-INL Would statsmodels needs to be updated now? Moving to the new class allows us to use newer versions of statsmodels, but the ARIMA class we're using has been there since 0.11.0 I think, so we shouldn't need to update right now. Is there a way for me to see why the checks are failing now? They were passing on here earlier, and all of the relevant tests still seem to be passing on my (Windows) machine. I was able to run some tests on a Linux machine, and I had a test fail due to a value falling just outside of a tolerance, so maybe some check tolerances need to be loosened a little? |
@j-bryan It looks like the ARMAParallel test is failing. Does it pass on your machine? |
@dylanjm Trying around on different operating systems I have access to here, ARMAparallel passes on Windows but fails on Mac and Linux (Ubuntu-based). It's failing with a pretty hairy-looking error being kicked out from ray (ValueError: buffer source array is read-only). I can get you the stack trace if you'd like. I'm also getting that (negligible) diff I mentioned for the ARMA/Basic test on Linux only. It passes on Windows and Mac. |
@j-bryan ooof 🙃
I think I can see the RAY error stack trace coming from the testing machines. I'm not quite sure how we should fix this. @joshua-cogliati-inl does this RAY error look familiar to you? |
Yes, this error means that something is being serialized (pickled) and deserialized that was not designed to be serialized. (How to fix on the other hand might be a challenge.) (A first guess would be modifying |
Hmmm that's a tricky one. I'll start looking at the ARMA ROM pickling, but this could take a sec to figure out. Some other tests seem to successfully pickle and unpickle the ROM, and it's only failing in the parallel test. In the meantime, let me know if you have any other ideas of what might be causing the error. |
Yes, it can take some time to figure out. I think the biggest hint is statsmodels/tsa/statespace/_initialization.pyx is roughly what is failing. |
FYI: the last time I saw this error message: scikit-learn/scikit-learn#21685 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. No major functionality is changed.
Looks like we are getting a failure in pickleTests: Here's the test name
Here's the stack trace:
|
Job Mingw Test on 9b2450d : invalidated by @joshua-cogliati-inl civet settings changed. |
Job Mingw Test on 9b2450d : canceled by @wangcj05 waiting for #1913 |
Job Mingw Test on 9b2450d : invalidated by @wangcj05 |
Job Mingw Test on 9b2450d : canceled by @wangcj05 |
@dylanjm Which tests are failing now? I've rerun the tests on my Mac and Windows machines, and everything is passing for me. |
@j-bryan It looks like the test is failing due to something being broken upstream. It looks like #1913 fixes this issue. If you're curious about the failing test it is:
Once that fix is merged, you'll have to update RAVEN, rebase, and then force push to get this PR to pass tests. |
Okay, I'll keep an eye out for that. Thanks! |
1 similar comment
It looks like some small numerical error is preventing this merge in the ARMA "Basic" test:
|
Hm, this causes HERON and FARM to fail: https://civet.inl.gov/job/1125079/:
So SupervisedLearning/ARMA.py", line 1391 probably needs to be updated. |
@@ -1382,7 +1388,7 @@ def writeXML(self, writeTo, targets=None, skip=None): | |||
root.append(targetNode) | |||
armaNode = xmlUtils.newNode('ARMA_params') | |||
targetNode.append(armaNode) | |||
armaNode.append(xmlUtils.newNode('std', text=np.sqrt(arma.sigma2))) | |||
armaNode.append(xmlUtils.newNode('std', text=arma.sigma)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is failing in HERON tests: https://civet.inl.gov/job/1125079/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is because we don't have a way to update the trained ARMAs that we use for testing as prerequisites to running the HERON cases, so if ever the ARMA ROM gets updated, we have to manually update those serialized testing ARMAs. This may not be avoidable, but we should make sure that @j-bryan can update those trained ARMAs in HERON immediately after merging this. Will that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is fine with me. (Is there written documentation on how to do this? I checked Troubleshooting and some other pages in the HERON wiki and didn't see how to update those ARMAs.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should be documented better. There's RAVEN XML files in the ARMA test folder along with the associated training data, so it should be a simple matter of running those training XML files with the new changes in RAVEN present.
@j-bryan this is ready to merge, but I want to make sure first that you'll be able to follow up with the HERON fixes that @joshua-cogliati-inl has noted. Let me know if there's questions on that. |
@PaulTalbot-INL I should be able to handle that. Just to check my understanding of the issue, some of the HERON (and FARM?) tests rely on ARMA models which need to be retrained. I’ll need to go and retrain those models manually using the new version of the ARMA ROM. Is that correct? |
That's right, the training workflows should all be there along with the data in |
Updates pickled ARMA ROMs due to changes from idaholab/raven#1896
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
#1872
What are the significant changes in functionality due to this change request?
Removes use of statsmodels.tsa.arima_model.ARMA and replaces it with statsmodels.tsa.arima.model.ARIMA, making additional changes as needed. Some attributes in the armaResultsProxy class (in ravenframework/SupervisedLearning/ARMA.py) were changed to better match the ARIMAResults object returned by the new ARIMA model class being used; these changes will break existing pickled ARMA ROMs!
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.raven/tests/framework/user_guide
andraven/docs/workshop
) have been changed, the associated documentation must be reviewed and assured the text matches the example.