Hop Time Fixes

[ansoncfit]

This PR fixes two problems converting trip patterns from the scenario editor UI to R5 modifications:

1. Speed conversion factors (an incorrect magic number)
2. Speeds and dwell times getting out of sync with hops #174

[codecov-io]

Codecov Report

Merging #173 into dev will decrease coverage by <.01%. The diff coverage is 0%.

@@             Coverage Diff              @@
##                dev     #173      +/-   ##
============================================
- Coverage     19.62%   19.61%   -0.01%     
  Complexity       75       75              
============================================
  Files            58       58              
  Lines          2125     2126       +1     
  Branches        187      188       +1     
============================================
  Hits            417      417              
- Misses         1689     1690       +1     
  Partials         19       19

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/conveyal/taui/models/ModificationStop.java	0% <0%> (ø)	0 <0> (ø)	:arrow_down:
...rc/main/java/com/conveyal/taui/models/Reroute.java	0% <0%> (ø)	0 <0> (ø)	:arrow_down:
.../java/com/conveyal/taui/models/AddTripPattern.java	21.42% <0%> (ø)	2 <0> (ø)	:arrow_down:
...va/com/conveyal/taui/models/AbstractTimetable.java	3.7% <0%> (ø)	1% <0%> (ø)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 200082e...20e82f0. Read the comment docs.

[abyrd]

@ansoncfit please review these changes - I couldn't add you as a reviewer since you created the PR.

[ansoncfit]

Thanks for the fix, @abyrd. We should note that this change AFFECTS RESULTS by slowing erroneously fast speeds to intended values in add-trip and reroute modifications (fixing the magic number), and in less straightforward ways for modifications that include both newly created stops and widely varying segment-specific speeds (fixing #174).

[abyrd]

On 3 Oct 2018, at 11:11, Anson Stewart notifications@github.com wrote:

• if (stop.autoGenerated) { Oh. Shouldn't this be if (!stop.autoGenerated)? The realStopIndex should be incremented if the stop is a realStop (i.e. not auto generated).

Correct. Thanks for catching that. I had used a special instance of String at first and switched to the Boolean later - looks like I got the operator wrong during that replacement.

This highlights the fact that we’re fixing features that we don’t regularly use and for which we don’t have tests, and which are relatively hard to test by hand. that’s not good - we wouldn’t know when this breaks again. I think the only reasonable solution is to add some automated tests. And possibly to remove or simplify some functionality to ensure complexity is aligned with the number of people maintaining it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[abyrd]

However, I'm working through what we'd need for automated testing and for testing modifications, we'd have to spin up a whole stack of components, get front end modification into the database, get the backend to take them out and recode them to R5 modifications, etc. This is all going to end up being tightly coupled to the current implementation, making it hard to improve/simplify that implementation without invalidating tests, and wasting effort invested in those tests.

I think for the time being that effort is best spent on making it easy to deploy to staging and maintaining the checklist of functionality to systematically test out by hand. The most straightforward way to test this functionality is just manual functional testing on a complete staging deployment.

So for this patch I really should have recorded a new manual test case for future re-use (which would have caught my negation mistake). I'll do that today before approving #176.

[abyrd]

For future reference, note that this PR was merged while still containing a bug, which @ansoncfit has repaired in follow up PR #176.