Added ability to allow the user to change the name of the integration variable. #874
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he name of the users choosing. Elapsed time of a phase has been renamed from time_phase to t_phase to reflect the fact that t is now the generic integration variable.
…looking for the wrong timeseries name when the linkage variable was the independent variable
…aints/objective/timeseries to support existing code with a deprecation warning.
…date introspection to search for appropriate name in constraints, objectives, rates, etc.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
johnjasa
approved these changes
Nov 24, 2022
| @@ -263,7 +264,7 @@ | |||
| "# timeODE.py\n", | |||
| "phase.set_time_options(fix_initial=True, fix_duration=True, duration_val=s_final,\n", | |||
| " targets=['curv.s'], units='m', duration_ref=s_final,\n", | |||
| " duration_ref0=10)\n", | |||
| " duration_ref0=10, name='s')\n", | |||
There was a problem hiding this comment.
At first glance, this name='s' confuses me. I will review the PR and see if it makes more sense. I'm leaving this note here so I remember to check back in here.
There was a problem hiding this comment.
I've left this alone because it was an external contribution. In my mind 's' is often used for arclength so it makes sense to me.
dymos/phase/phase.py
Outdated
| as a timeseries output. If a name, it be one of 'time', 'time_phase', one of the states, controls, | ||
| control rates, or parameters, in the phase, the path to an output variable in the ODE, or a glob pattern | ||
| matching some outputs in the ODE. | ||
| as a timeseries output. If a name, it be one of the integration variable, 'time_phase', one of the states, |
There was a problem hiding this comment.
"If a name, it be one of" -> "If a name, it should be one of"? or maybe a word is missing in here
|
|
||
| # Solve for the optimal trajectory | ||
| p.run_driver() | ||
| for time_name in ('time', 'elaspsed_time'): |
There was a problem hiding this comment.
should change 'elaspsed' to 'elapsed' in three places
| @@ -63,7 +63,7 @@ def configure_time(self, phase): | |||
| Configure the inputs/outputs on the time component. | |||
|
|
|||
| This method assumes that target introspection has already been performed by the phase and thus | |||
| options['targets'], options['time_phase_targets'], options['t_initial_targets'], | |||
| options['targets'], options['timw_phase_targets'], options['t_initial_targets'], | |||
…introspection work
…was being performed.
robfalck
force-pushed
the
rename_time2
branch
from
55dbd8e to
a81c083
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The user may now change the name of the integration variable, which defaults to "time" for backward compatibility.
The elapsed time of the phase can now be referenced as
{time_name}_phase(it was previouslytime_phase).Changed the name of the integration variable to
sin the race car example and documentation.Added documentation of renaming time.
Related Issues
Backwards incompatibilities
Internally, the names of time and time_phase coming out of the TimeComp are now
tandt_phase. This shouldn't affect many users as they should be pulling these values from the timeseries instead.New Dependencies
None