Fix for trajectory parameters that had no valid target but raised no meaningful error. #744
Conversation
…ctory parameters are not properly defined. Even found a few existing issues in tests.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -379,7 +379,8 @@ | |||
| "# In this case, we connect 'm' to pre-existing input parameters\n", | |||
| "# named 'mass' in each phase.\n", | |||
| "traj.add_parameter('m', units='kg', val=1.0,\n", | |||
| " targets={'ascent': 'mass', 'descent': 'mass'}, static_target=True)\n", | |||
There was a problem hiding this comment.
How did these tests pass before? Is the default value of mass the same as the value set in the problem?
There was a problem hiding this comment.
Honestly not sure, that must be the case.
| p.driver.options['optimizer'] = OPTIMIZER | ||
| p.driver.declare_coloring() | ||
|
|
||
| if transcription == 'gauss-lobatto': |
There was a problem hiding this comment.
were you going to parameterize this test for both transcriptions?
| phase.add_parameter('S', val=49.2386) | ||
|
|
||
| # Unit introspection for traj params. This doesn't work. | ||
| traj.add_parameter('Isp', val=1600.0, targets={'phase0': 'Isp'}) |
There was a problem hiding this comment.
I'm confused that this one raises an error message. Isp is a variable in the phase (though not a parameter.) BTW, I ran my LEAPS stuff with your branch, and it works. This is my add_parameter:
for name in aero_inputs:
traj.add_parameter(name, units=MetaData[name]['units'],
targets={'climb': [name]}, shape=1, static_target=True)
I guess it only works for me because static_target is True, but I am not sure why that makes a difference here.
There was a problem hiding this comment.
So it's a very subtle difference, which is why I've come to prefer forcing users to explicitly add parameters to phases and then let them be targed by trajectory parameters.
In targets={'phase0': 'Isp'}, 'Isp' is a string so it's assumed to be a parameter. If 'Isp' was provided in a sequence (['Isp']), then it would assume its to find it in the ODE. Some of this implementation dates back to the time before we could do introspection.
I think things are a lot simpler with being explicit here: Parameters provide an input to get external values into the phase, and we can use trajectory parameters to connect an external output to a single target (the trajectory parameter) and have connect to all relevant parameters in the phase.
I'd be in favor of ONLY documenting that way, and deprecating the use of a sequence for targets.
Summary
If a user tried to create a trajectory parameter that was intended to be connected to phase parameters, but someone made an erroneous target specification, Dymos would not raise an error and run without the user's intended behavior.
Related Issues
Backwards incompatibilities
This may find issues with trajectory parameters that were previously missed.
New Dependencies
None