-
Notifications
You must be signed in to change notification settings - Fork 37
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
Updates pickled ARMA ROMs #195
Conversation
…ences due to changes in ARMA
@PaulTalbot-INL HERON tests pass, but FARM tests do not. Looks like FARM has an ARMA that needs to be retrained to pass the heron_validator tests. Can this be merged anyway? |
It looks like FARM is using old ARMA files too ... I'm a little surprised they're using trained ARMAs that aren't in HERON. Let me look at their repo real quick and see if I can make sense of it. |
Yes, they're using ARMAs in their repo, which also need to be retrained. I will send Haoyu a note about this, but it shouldn't stop this PR ... although it looks like it does. Hm. By our SQA, you only need to demonstrate that the plugin works with the current RAVEN and any plugins you depend on, which doesn't include FARM for HERON, so technically by the SQA this is okay to merge. We probably need to tinker with the test to be clearer on what plugins it uses, instead of "all". |
Okay. I talked with a couple people, and I think the right fix is to update the script that runs the tests to make sure we only check the plugins that we depend on, not "all". I'll come up with a fix and provide that to @j-bryan so you can include it in this PR, then we should be good to go. However, we did fail to notify all plugin owners of the need to update ARMA, so we need to inform ANL specifically of that. I can send that email as well. |
Of course, because nothing is ever that simple, I have to get access to a private repo to make a change to a standard script that we use to install things as a RAVEN plugin. Since that's going to take a bit, I will note here that the tests we need to pass for this have passed, and approve an exception to the automated tests passing based on that. @dgarrett622 you can merge this if you feel the other requirements are satisfied. |
Okay, everything looks good for the HERON tests and no real code to review. I'll get this merged in. Thanks @j-bryan! |
@PaulTalbot-INL I noticed the failing FARM test and submitted a PR to them to update that at the same time I made this one, so at the very least they should be aware of the issue. |
Perfect! Thank you. |
Pull Request Description
What issue does this change request address?
#194
What are the significant changes in functionality due to this change request?
No significant changes in functionality. Updates pickled ARMA ROMs and regolds one test (integration_tests/mechanics/labels/Labels) that had very slight differences in some values due to the recent RAVEN changes.
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.