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

Updates pickled ARMA ROMs #195

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Updates pickled ARMA ROMs #195

merged 1 commit into from
Aug 3, 2022

Conversation

j-bryan
Copy link
Contributor

@j-bryan j-bryan commented Aug 3, 2022


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.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large tes.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added, the the analytic documentation must be updated/added.
  • 9. If any test used as a basis for documentation examples have been changed, the associated documentation must be reviewed and assured the text matches the example.

@dgarrett622
Copy link
Collaborator

@j-bryan for RAVEN compatibility issues like this one, in the future you can just link to issue #74 instead of creating a separate issue. It might save some time on your end 😄

@dgarrett622 dgarrett622 linked an issue Aug 3, 2022 that may be closed by this pull request
10 tasks
@dgarrett622
Copy link
Collaborator

@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?

@PaulTalbot-INL
Copy link
Collaborator

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.

@PaulTalbot-INL
Copy link
Collaborator

PaulTalbot-INL commented Aug 3, 2022

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".

@PaulTalbot-INL
Copy link
Collaborator

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.

@PaulTalbot-INL
Copy link
Collaborator

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.

@dgarrett622
Copy link
Collaborator

Okay, everything looks good for the HERON tests and no real code to review. I'll get this merged in. Thanks @j-bryan!

@dgarrett622 dgarrett622 merged commit d420aeb into idaholab:devel Aug 3, 2022
@j-bryan
Copy link
Contributor Author

j-bryan commented Aug 3, 2022

@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.

@PaulTalbot-INL
Copy link
Collaborator

Perfect! Thank you.

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.

[DEFECT] Changes to ARMA ROM in RAVEN has broken saved ROM pk files in tests
3 participants