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

Add allowMissing kwarg to loadOperator #538

Merged
merged 1 commit into from Jan 17, 2022
Merged

Conversation

onufer
Copy link
Contributor

@onufer onufer commented Jan 14, 2022

Description

Adds kwarg to loadOperator to support more easily loading databases that have parameters not defined within the current ARMI version. This is helpful for backward compatibility when post-processing a database that had a param that was removed from ARMI .


Checklist

  • Tests have been added/updated to verify that the new or changed code works.
  • Docstrings were included and updated as necessary
  • The code is understandable and maintainable to people beyond the author
  • There is no commented out code in this PR.

If user exposed functionality was added/changed:

  • Documentation added/updated in the doc folder.
  • New or updated dependencies have been added to setup.py.

Adds this kwarg to loadOperator to support more easily loading databases
that have parameters not defined within the current ARMI version. 
This is helpful for backward compatibility when post-processing database 
that had a param that was removed from ARMI .
@john-science
Copy link
Member

I'll wait for the unit tests to finish running before approving.

But this looks like a very reasonable change.

It might be nice to have a unit test that flexes this for allowMissing" = TrueandFalse. There is already one unit test that uses loadOperator()`:

o = armi_init(fName=TEST_INPUT_TITLE + ".yaml")
locsInput, locsDB = {}, {}
loadLocs(o, locsInput)
o.operate()
o2 = db.loadOperator(TEST_INPUT_TITLE + ".h5", 0, 0)

@onufer
Copy link
Contributor Author

onufer commented Jan 14, 2022

@john-science, Yes, the main reason i didn't add a test is that to trully test the feature I would need a database that had params that don't exist in the framework. I dont really want to commit another database to the framework, and dont want to write a test that doesn't actually test the feature. Open to feedback though.

@john-science
Copy link
Member

@onufer Is it true that we need a whole need database to test this?

Couldn't we just create a new test YAML file that lists the parameters we need, to exclude on of the parameters in the default test database?

Sorry, maybe I'm missing something.

@onufer
Copy link
Contributor Author

onufer commented Jan 17, 2022

@john-science, you can test the method signature but not the feature, since the w/o a new db it wouldn't fail either way.

@onufer onufer merged commit e7aa498 into master Jan 17, 2022
@john-science john-science deleted the loadOperator-allowsMissing branch January 17, 2022 18:37
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
Adds this kwarg to loadOperator to support more easily loading databases
that have parameters not defined within the current ARMI version. 
This is helpful for backward compatibility when post-processing database 
that had a param that was removed from ARMI.
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.

None yet

2 participants