-
Notifications
You must be signed in to change notification settings - Fork 254
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
Poem 047 Implementation 1 #2005
Conversation
test_poem_047_i1.py
Outdated
|
||
if __name__ == '__main__': | ||
|
||
p = Parabola() |
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.
Can you refactor this as a test using a unittest.TestCase? That's how we do our testing in OpenMDAO. See any of our other test_* files for examples.
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.
Test Refactor should now be complete. I left it in the root folder as I'm not sure when you want to stash it (or even if it's a test you'd want to keep around).
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.
since the code being tested is in openmdao/core/system.py
, we would want this test in openmdao/core/tests/
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 moved it there and renamed to match what it's testing (rather than just referencing the POEM number)
Addressed swryans comment
Test Code refactor as per Bret
openmdao/core/system.py
Outdated
if self._problem_meta['model_ref']() is not None: | ||
model_ref = self._problem_meta['model_ref']() | ||
else: | ||
model_ref = None |
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.
the if/else
here is unnecessary?
this is equivalent to model_ref = self._problem_meta['model_ref']()
?
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.
Whoops, yep, good point. Should be corrected now
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.
actually, I think I see what you intended to do here... did you mean this?
if self._problem_meta is not None:
model_ref = self._problem_meta['model_ref']()
else:
model_ref = None
since the following statement expects model_ref
to exist even if _problem_meta
is None
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.
Sorry, I think I jumped to changing things without thinking about what I was doing. Yes, what you've written above is correct and should have the behavior I intended. I've updated the latest commit accordingly.
Removed unnecessary if
Removed file from root level
Updated to ensure model_ref is assigned
Thanks for your effort on this @andrewellis55 ... you can ignore the GitHub (
Thanks again for your contribution... |
PEP violation fixed
Pep adherance
PEP fix
End of File Blank Line
@swryan Thanks for you help reviewing the changes. The PEP issues should be fixed with the latest commit |
Summary
Implementation 1 from POEM 047. Enables components to access their inputs and outputs without dependency on the problem metadata
Related Issues
Backwards incompatibilities
None
New Dependencies
None