Use a record instead of a pair for profile segments#434
Merged
mattdailis merged 2 commits intodevelopfrom Nov 18, 2022
Merged
Conversation
mattdailis
added
the
refactor
mattdailis
force-pushed
the
refactor/profile-segment-record
branch
from
d7e4352 to
c8b6c6a
Compare
JoelCourtney
requested changes
Nov 9, 2022
mattdailis
force-pushed
the
refactor/profile-segment-record
branch
from
c8b6c6a to
5c990bf
Compare
JoelCourtney
approved these changes
Nov 9, 2022
mattdailis
force-pushed
the
refactor/profile-segment-record
branch
from
5c990bf to
08ad7a0
Compare
camargo
changed the title
camargo
approved these changes
Nov 15, 2022
mattdailis
force-pushed
the
refactor/profile-segment-record
branch
from
08ad7a0 to
f0960c6
Compare
camargo
added
the
breaking change
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description
Profile segments in simulation results are currently represented as a
Pair<Duration, Dynamics>- it is difficult to tell from this representation what the meaning of the "Duration" should be. By convention, we interpret theDurationto be theextentof the Profile Segment - i.e. the length of this segment. Other plausible interpretations would be the offset from the start of the plan, as well as the length of the previous segment.This PR introduces a
ProfileSegmentrecord to make it clear which interpretation is being used, and to pave the way for changing that interpretation in the near future. (The current interpretation is somewhat incompatible with streaming - since as it stands you cannot emit a segment until its end time is known).This PR updates
SimulationResultsas well as all uses of profiles that havemerlin-driveras a dependency. It draws the line at theconstraintspackage, which does not depend onmerlin-driver.As @JoelCourtney pointed out, we have numerous representations of profiles, and it's hard to keep them all straight - while this PR does not reduce the number of representations, it hopefully makes one of those representations a little easier to work with.
Verification
I spun up a local aerie and ran constraints checking, just to make sure that pipeline still works. Otherwise, I consider the fact that the code compiles, and that the tests pass on this PR to be sufficient verification.
Documentation
The
ProfileSegmentclass has associated javadoc, but aside from that no documentation has been updated.Future work
Future work: simulation results streaming