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

Added capability for IVCs to be instantiated with a list of variables #3197

Closed
wants to merge 9 commits into from

Conversation

johnjasa
Copy link
Member

Summary

There's an error message for IndepVarComp that says you can use an iterable at instantiation, but I don't think the code was there anymore.
I added the option to do that here and two tests accordingly.

Shout-out to @crecine for trying this and saying "hey should this work?"

Related Issues

  • Resolves #

Backwards incompatibilities

None

New Dependencies

None

@@ -39,6 +39,16 @@ def __init__(self, name=None, val=1.0, **kwargs):
if isinstance(name, str):
super().add_output(name, val, **kwargs)

elif isinstance(name, (list, tuple)):
for tup in name:
if len(tup) == 2:
Copy link
Member

Choose a reason for hiding this comment

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

You might want to verify here that tup is actually a tuple, or at least not a string. Otherwise in cases where the user gives you a list/tuple of strings, you'll get weird error messages. I could see a user just providing a list of strings and assuming those variables would all have default values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I've modified this and pushed up!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the docstring for name needs to be updated for these new possibilities...

Also maybe some kind of error checking on the tup values (i.e. tup[0] must be a string, tup[2] must be a dict?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Steve, I've updated the docstring and added type checking!

@coveralls
Copy link

coveralls commented Apr 17, 2024

Coverage Status

coverage: 88.705% (-0.2%) from 88.94%
when pulling adca856 on johnjasa:update_ivc
into 3eab709 on OpenMDAO:master.

@@ -39,6 +39,16 @@ def __init__(self, name=None, val=1.0, **kwargs):
if isinstance(name, str):
super().add_output(name, val, **kwargs)

elif isinstance(name, (list, tuple)):
for tup in name:
if len(tup) == 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the docstring for name needs to be updated for these new possibilities...

Also maybe some kind of error checking on the tup values (i.e. tup[0] must be a string, tup[2] must be a dict?)?

@swryan
Copy link
Contributor

swryan commented Apr 24, 2024

@johnjasa, looks like you just need to clean up some PEP8 stuff on this..

@naylor-b, I think your comment has been addressed?

@johnjasa
Copy link
Member Author

Thanks for the ping, @swryan, I just fixed the pep issues

@johnjasa johnjasa closed this Apr 24, 2024
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

4 participants