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

Impl. PR for POEM_029 - Retrieval of IO Variable Metadata #1577

Merged
merged 41 commits into from
Aug 13, 2020

Conversation

naylor-b
Copy link
Member

@naylor-b naylor-b commented Jul 30, 2020

Summary

Adds the get_io_metadata method to System and modifies list_inputs and list_outputs to use it internally.
This is only an initial implementation and will have further updates prior to POEM_029 approval.

Related Issues

Backwards incompatibilities

  • Behavior of list_inputs/list_outputs is changed slightly. Data is now returned as a dict rather than a list. Variable names associated with metadata in the returned dict always use relative names and always include variables from all MPI processes. Also, for distributed variables, the full variable will be returned in the metadata dict (which will now match what is written to the stream).

New Dependencies

None

@naylor-b naylor-b added the Pending POEM Acceptance Do not merge. This PR is attached to a POEM that is not yet accepted. label Jul 30, 2020
@project-bot project-bot bot added this to In progress in OpenMDAO Dev [Read only] Jul 30, 2020
@robfalck robfalck self-requested a review August 7, 2020 18:34
@project-bot project-bot bot moved this from In progress to Under review in OpenMDAO Dev [Read only] Aug 7, 2020
Copy link
Member

@JustinSGray JustinSGray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me, but the one big test I noted has some pretty tough to decifer code. Can you at least add a few doc strings or comments?

I know you were reducing code duplication, but I was having trouble decifering the asserts. I think other developers would struggle to discern the correct expected behavior if the test broke and they had to fix it.


def _descendents(mysystem, sysiter):
"""
Iterate over system ancestors, but include only mysystem's descendents.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest clarification: `Filter given iterator of system paths to include only mysystem's descendents

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import numpy as np
import openmdao.api as om

class FlightDataComp(om.ExplicitComponent):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting component names here. Did these come from some reduced dymos test case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume so. This test was already there and I just modified it.

OpenMDAO Dev [Read only] automation moved this from Under review to Reviewer approved Aug 11, 2020
openmdao/core/system.py Outdated Show resolved Hide resolved
@swryan swryan removed the Pending POEM Acceptance Do not merge. This PR is attached to a POEM that is not yet accepted. label Aug 13, 2020
@swryan swryan merged commit 765e0c7 into OpenMDAO:master Aug 13, 2020
OpenMDAO Dev [Read only] automation moved this from Reviewer approved to Done Aug 13, 2020
swryan added a commit that referenced this pull request Aug 14, 2020
Impl from PR #1577 but reverted return value of list_inputs/list_outputs back to lists
@swryan swryan added the POEM Indicates an issue/PR that references a POEM label Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
POEM Indicates an issue/PR that references a POEM
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants