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

Support for src_indices applied at multiple levels in the system tree #1760

Merged
merged 42 commits into from Nov 11, 2020

Conversation

naylor-b
Copy link
Member

@naylor-b naylor-b commented Nov 9, 2020

Summary

You can now specify src_indices in connect as well as at various group levels via the promotes call.

Related Issues

Backwards incompatibilities

Setting src_indices in add_input is now deprecated and will be an error in a later release.

New Dependencies

None

@project-bot project-bot bot added this to In progress in OpenMDAO Dev [Read only] Nov 9, 2020

prob.run_model()

# we gave 'G1.x' units of 'm' in the set_input_defaults call
assert_near_equal(prob['G1.x'], 2.0, 1e-6)
assert_near_equal(prob['G1.x'], np.ones(3) * 2.0, 1e-6)
Copy link
Member

Choose a reason for hiding this comment

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

Was this a bug before? (i.e., was the target pulling values beyond the bounds of the array?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a bug, but it wasn't pulling values from beyond the array bounds. The auto_ivc was the correct size before as well, but the promoted input 'G1.x' was getting mapped to use the src_indices for G1.C1.x so a get_value call was just returning part of the source value.

OpenMDAO Dev [Read only] automation moved this from In progress to Reviewer approved Nov 10, 2020
@swryan swryan assigned swryan and unassigned swryan Nov 10, 2020
@swryan swryan requested a review from robfalck November 10, 2020 21:07
@project-bot project-bot bot moved this from Reviewer approved to Under review in OpenMDAO Dev [Read only] Nov 10, 2020
OpenMDAO Dev [Read only] automation moved this from Under review to Reviewer approved Nov 11, 2020
@swryan swryan merged commit 3197047 into OpenMDAO:master Nov 11, 2020
OpenMDAO Dev [Read only] automation moved this from Reviewer approved to Done Nov 11, 2020
@joanibal
Copy link

joanibal commented Jan 4, 2021

Hi all,

I want to understand why adding src_indices in add_input is now deprecated.

After looking at PR #1709 it looks like it was initially unclear how to integrate the src_indices used in add_input, and those used in promotes or connect.
Based on the last comment in the PR by @Kenneth-T-Moore, it sounds like there may have been another conversation offline.

After discussion, we determined a better way to do this inside a dymos model.

So, I was wondering if you all had anymore information as to why adding src_indices to a promotes or connect is now the preferred way. And furthermore, if this new method of adding src_indices has any benefits I should be aware of when rewriting the models that originally used src_indices in add_input.

@naylor-b naylor-b deleted the nested_src_indices branch January 4, 2021 20:04
@JustinSGray
Copy link
Member

JustinSGray commented Jan 4, 2021

its more of a logical reason, than anything else ... here is how I see it:

Strictly speaking, a component is best left atomic. It shouldn't need to know about anything around it. The size of an input is n and it doesn't matter how those n values get there. They just do. So src_indices doesn't really make sense there IMO.

The src_indices are associated with a connection. The connection is what maps an output (src) to the input. Hence the src indices are best associated during some kind of a connection operations (i.e. connect, promote).

This may seem like a subtle distinction, but it tuns out to matter in a weird case we discovered for Dymos. And down the rabit hole we go...

You can use src indices for more than just grabbing slices of arrays. You can also use them to create broadcast duplicates (i.e. src_indices=[0,0,0,0,0,0,0,0, 1,1,1,1,1,1,1,]). why would you do this? In Dymos, the ODE components/groups are written with inputs that are sometimes time-varying and sometimes not (maybe you want to set one single thrust for a whole phase one time, and another time you want thrust to be time varying). To keep things generic, the ODE is coded with a vector input for this. Then dymos handles the details via some of its API methods.

If you are going to use it as a time-invariant control, then maybe that value is something you want to compute upstream of dymos and pass into it. In that case, it makes sense that the computed value is scalar. You want to connect the scalar output to the vector input, and you can do that with src_indices broadcasting.

That is the simple use case. Now, here is the weird corner case. What if the single thrust value you want to use is itself a sub-set of some vector. For the sake of argument, lets say there is some length 5 vector of thrusts and you want the 3rd one. So on the boundary of dymos, you have a promoted input that is supposed to look like a scalar. At that level, you want to specify a connection with a src indices like that pulls out the 2nd value( i.e. src_indices=[3]). However, the next level down, Dymos needs to take that value and convert it to this: [3,3,3,3,3,3,3,3,3,3,3]. There is a catch though, because at the time that Dymos group was doing its setup/configure it knew about the broadcast connection but all it knew was that there was a scalar on the boundary. So inside dymos it did a promote with src_indices=[0,0,0,0,0,0,0,0,0,0,0]. So OpenMDAO has to step in, look at the top level connect, and the promote that was inside dymos and resolve the two of them to get the correct value.

I know thats a bit confusing, and probably needs some kind of diagram ...sorry. The point is this, this case of nested src_indices is useful when you want to change the size of something as you promote it up. OpenMDAO can resolve all of these nested src_indices (if possible), but that means that the src_indices are really associated with the promote/connect.

Now, because we have an older api that allows users to specify src_indices during add_input, we have one more problem. its not possible for users to specify them in both add_input and during the promote/connect at the group level. its also possible that these will conflict. Before we allowed nesting, the conflicts were easy to detect and throw an error. However, now there are many cases where the situation is ambiguous and its not clear if we are supposed to do some kind of mapping or not.

So the decision was made to deprecate the old API. You can still use it, though for the Mphys use case I think there is no difference between doing it at the group level or not. If you dig into it and find that group level isn't good, we can have a discussion about un-depracating it. We'll just have to come up with very clear error conditions in that situation.

@joanibal
Copy link

joanibal commented Jan 4, 2021

Thanks for the explanation!

That really cleared things up for me.

@JustinSGray
Copy link
Member

sorry for the very long diatribe response. This nested src_indices issue was super messy, and it took us weeks of talking in circles to sort it out. I think OM is better for it, and for the moment I stand by our deprecation. However, if you find it messed things up for you don't hesitate to talk to us about it. We'll dive back in if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6 participants