fix: False positives for StopTimeTimepointWithoutTimesNotice #1044

lionel-nj · 2021-10-26T19:50:02Z

Summary:

This PR provides support to no longer throw false positives in TimepointValidator.

Expected behavior:

sample data 1 (no timepoint column)

stop_sequence	arrival_time	departure_time
1	00:00:00	00:00:00
2
3
4	00:10:00	00:10:00

MissingTimepointColumnNotice should be issued once.

sample data 2 (timepoint column + no empty values)

stop_sequence	arrival_time	departure_time	timepoint
1	00:00:00	00:00:00	1
2	00:02:00	00:02:00	0
3	00:08:00	00:08:00	0
4	00:10:00	00:10:00	1

No notice

sample data 3 (timepoint with no times)

stop_sequence	arrival_time	departure_time	timepoint
1	00:00:00	00:00:00	1
2			1
3	00:08:00	00:08:00	0
4	00:10:00	00:10:00	1

StopTimeTimepointWithoutTimesNotice should be issued for the 2nd row.

sample data 4 (approximate, with times)

stop_sequence	arrival_time	departure_time	timepoint
1	00:00:00	00:00:00	1
2	00:02:00	00:02:00
3	00:08:00	00:08:00
4	00:10:00	00:10:00	1

MissingTimepointValueNotice should be issued for the 2nd and 3rd row.

sample data 5 (approximate, with no times)

stop_sequence	arrival_time	departure_time	timepoint
1	00:00:00	00:00:00	1
2
3
4	00:10:00	00:10:00	1

MissingTimepointValueNotice should be issued for the 2nd and 3rd row.

Please make sure these boxes are checked before submitting your pull request - thanks!

Run the unit tests with gradle test to make sure you didn't break anything
Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
Linked all relevant issues
~~[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)~~

…r a single record.

maximearmstrong

Few questions and comments in-line before approval :)

RULES.md

docs/NOTICES.md

maximearmstrong · 2021-10-29T23:03:35Z

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

 * </ul>
 */
 @GtfsValidator
 public class TimepointTimeValidator extends SingleEntityValidator<GtfsStopTime> {

  @Override
  public void validate(GtfsStopTime stopTime, NoticeContainer noticeContainer) {
+    if (!stopTime.hasTimepoint()) {


Does the validator fill the column with a default value if it is not in the dataset, or are we not entering this TimepointTimeValidator?

Does the validator fill the column with a default value if it is not in the dataset,

Yes, default is 1.

Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>

…r into fix/timepoint

isabelle-dr · 2021-11-01T13:19:53Z

One question here:
If the Sample data 1, it's written

MissingTimepointValueNotice should be issued for the 2nd and 3rd row.

It's not exactly what is mentioned here, is this a typo in the description of this PR? We talked about throwing the MissingTimepointValueNotice only if the column is present.

Also: do you need real datasets that fall in those different categories to test this PR?

lionel-nj · 2021-11-01T15:24:09Z

It's not exactly what is mentioned here, is this a typo in the description of this PR? We talked about throwing the MissingTimepointValueNotice only if the column is present.

Indeed, this will be fixed in the next commits. Thanks for flagging that.

Also: do you need real datasets that fall in those different categories to test this PR?

I'll use the one you flagged in this document, thanks.

lionel-nj · 2021-11-01T20:43:16Z

@maximearmstrong PTAL - the rule logic has been changed a bit to generate MissingTimepointColumnNotice when a dataset has no field timepoint in GTFS file stop_times.txt.

At present, out of ~1200 datasets on the MobilityDatabse:

44 datasets trigger StopTimeTimepointWithoutTimesNotice;
560 datasets trigger MissingTimepointColumnNotice (legacy datasets);
125 datasets trigger MissingTimepointValueNotice.

isabelle-dr · 2021-11-01T21:42:45Z

That StopTimeTimepointWithoutTimesNotice number makes a lot more sense!

The number of MissingTimepointColumnNotice is surprisingly high.
After looking at a few datasets that trigger it, I found a false positive 😩, here it the dataset.

isabelle-dr

Could you rename the notice names? :)

MissingTimepointColumn -> MissingTimePointColumnNotice
MissingTimepointValue -> MissingTimepointValueNotice

RULES.md

docs/NOTICES.md

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

asvechnikov2 · 2021-11-07T23:23:02Z

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

    }
  }

  private boolean isTimepoint(GtfsStopTime stopTime) {
-    return stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);
+    return stopTime.hasTimepoint() && stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);


I see, in this case should we remove default value instead?

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

asvechnikov2 · 2021-11-07T23:31:44Z

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java

+            .setDepartureTime(null)
+            .setStopId("stop id 0")
+            .setStopSequence(2)
+            .setTimepoint((Integer) null)


Thanks for the explanation!

It looks like we may need to create a feature request to have an ability to specify test entities in the csv format, otherwise, we're trying to repeat logic of auto generated code, which may change and we end up with wrong tests.

- clarify docstrings - remove use of Strings.format()

isabelle-dr · 2021-11-16T13:15:10Z

@asvechnikov2 can we get a review so we can merge this PR?
See this comment for what we are proposing to do.

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

barbeau · 2021-11-16T15:14:50Z

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java

+            .setDepartureTime(GtfsTime.fromSecondsSinceMidnight(580))
+            .setStopId("stop id 2")
+            .setStopSequence(2)
+            .setTimepoint(1)


Is it right to set timepoint equal to 1 for this test? From the method name I expected both to be set to null. If it's intended, IMHO it's a bit awkward as it's not real data - you'd never have two records for stop times where one has the column and the other does not.

Follow up question after looking at other tests - how do you differentiate between "no column" and "no value" in the unit tests?

Is the missing timepoint value in CsvHeader header what triggers MissingTimepointColumnNotice (even if you call .setTimepoint(1) on one of the StopTime records)?

Is setting .setTimepoint(null) what is used to define a missing value (even if the timepoint value is included in header)?

I'd suggest clearly documenting this behavior in the unit tests as right now that's not 100% clear (to me at least), even following the discussion thread in https://github.com/MobilityData/gtfs-validator/pull/1044/files#r742464479. And I'd avoid any confusing data that won't exist in the real world, like a missing column but a record with a timepoint value (i.e., this test currently).

Follow up question after looking at other tests - how do you differentiate between "no column" and "no value" in the unit tests?

no column -> these tests use he legacy header

no value -> these tests use the regular header, with null timepoint value.

Is the missing timepoint value in CsvHeader header what triggers MissingTimepointColumnNotice (even if you call .setTimepoint(1) on one of the StopTime records)?

Yes.

Is setting .setTimepoint(null) what is used to define a missing value (even if the timepoint value is included in header)?

Exactly.

I'd suggest clearly documenting this behavior in the unit tests as right now that's not 100% clear (to me at least), even following the discussion thread in https://github.com/MobilityData/gtfs-validator/pull/1044/files#r742464479. And I'd avoid any confusing data that won't exist in the real world, like a missing column but a record with a timepoint value (i.e., this test currently).

👍🏾 I removed the non-necessary (and misleading records) in e33c960. And added additional documentation in 84926f4.

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java

github-actions · 2021-11-16T17:05:20Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

barbeau

Thanks @lionel-nj, a suggestion for docs and a few more comments in-line.

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java

barbeau

👍 LGTM

asvechnikov2

@asvechnikov2 can we get a review so we can merge this PR? See this comment for what we are proposing to do.

LGTM! Sorry, I was sure that I'd removed "requested changes" review and the PR had been merged already.

asvechnikov2 · 2021-11-16T22:39:22Z

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java

    }
  }

  private boolean isTimepoint(GtfsStopTime stopTime) {
-    return stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);
+    return stopTime.hasTimepoint() && stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT);


nit: I'd add a comment to clarify the behaviour, here and probably in the class docstring as well.

lionel-nj added 3 commits October 26, 2021 15:19

Emit a notice when timepoint = 1 AND times are missing

a72b888

Emit a notice (WARNING) when stop_times.timepoint value is missing fo…

febb426

…r a single record.

update javadocs

c2fa449

lionel-nj self-assigned this Oct 26, 2021

lionel-nj linked an issue Oct 26, 2021 that may be closed by this pull request

False positives for StopTimeTimepointWithoutTimesNotice #954

Closed

lionel-nj requested review from aababilov, isabelle-dr and maximearmstrong October 26, 2021 19:53

isabelle-dr added this to the v3.0.0 milestone Oct 27, 2021

lionel-nj requested a review from barbeau October 28, 2021 18:57

maximearmstrong suggested changes Oct 29, 2021

View reviewed changes

lionel-nj and others added 3 commits November 1, 2021 09:18

Merge branch 'master' into fix/timepoint

b3ec654

apply suggestion from code review

fc14880

Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>

Merge branch 'fix/timepoint' of github.com:MobilityData/gtfs-validato…

d44332a

…r into fix/timepoint

isabelle-dr mentioned this pull request Nov 1, 2021

Get stats after timepoint fix #1047

Closed

lionel-nj added 6 commits November 1, 2021 11:24

clarify documentation

fa647ef

modify rule logic to flag legacy datasets not using timepoint column

8e91071

refactor StopTimeTimepointWithoutTimesNotice for code clarity

c1d8fc3

update rules documentation

7497e16

issue MissingTimepointValueNotice in rule logic

bab59bf

modify rule logic

df1f702

lionel-nj requested review from maximearmstrong and asvechnikov2 November 1, 2021 20:43

lionel-nj added 2 commits November 2, 2021 10:01

touch

a2b5e3b

persist metadata

ddf420b

isabelle-dr requested changes Nov 2, 2021

View reviewed changes

lionel-nj mentioned this pull request Nov 5, 2021

chore: upgrade StopTimeTimepointWithoutTimesNotice to ERROR #1022

Closed

4 tasks

asvechnikov2 requested changes Nov 7, 2021

View reviewed changes

apply suggestions from code review

b9dd33f

- clarify docstrings - remove use of Strings.format()

lionel-nj mentioned this pull request Nov 10, 2021

Standardize on the use of @DefaultValue annotation #1059

Open

Merge branch 'master' into fix/timepoint

22c59aa

lionel-nj mentioned this pull request Nov 10, 2021

Downgrade InvalidPhoneNumberNotice #1060

Closed

isabelle-dr mentioned this pull request Nov 10, 2021

Analysis of validation results on the mobility database #1041

Closed

7 tasks

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/main/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidator.java Outdated Show resolved Hide resolved

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java Show resolved Hide resolved

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java Outdated Show resolved Hide resolved

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java Outdated Show resolved Hide resolved

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java Outdated Show resolved Hide resolved

barbeau reviewed Nov 16, 2021

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/validator/TimepointTimeValidatorTest.java Outdated Show resolved Hide resolved

lionel-nj added 2 commits November 16, 2021 11:27

remove StopTime record where not needed

e33c960

add context to notice upgrade [acceptance test skip]

dc6462c

lionel-nj added 2 commits November 16, 2021 12:08

apply goJF [acceptance test skip]

0a74e40

additional documentation in unit tests [acceptance test skip]

84926f4

lionel-nj requested a review from barbeau November 16, 2021 17:19

barbeau reviewed Nov 16, 2021

View reviewed changes

apply suggestions from code review [acceptance test skip]

35f47ce

lionel-nj requested a review from barbeau November 16, 2021 20:32

barbeau approved these changes Nov 16, 2021

View reviewed changes

lionel-nj merged commit 57689d7 into master Nov 16, 2021

lionel-nj deleted the fix/timepoint branch November 16, 2021 21:23

asvechnikov2 reviewed Nov 16, 2021

View reviewed changes

lionel-nj mentioned this pull request Nov 19, 2021

feat: acceptance tests #848

Merged

4 tasks

isabelle-dr mentioned this pull request Mar 16, 2022

Extend timepoint time validation #803

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: False positives for StopTimeTimepointWithoutTimesNotice #1044

fix: False positives for StopTimeTimepointWithoutTimesNotice #1044

lionel-nj commented Oct 26, 2021 •

edited

Loading

maximearmstrong left a comment

maximearmstrong Oct 29, 2021

lionel-nj Nov 1, 2021

isabelle-dr commented Nov 1, 2021

lionel-nj commented Nov 1, 2021 •

edited

Loading

lionel-nj commented Nov 1, 2021 •

edited

Loading

isabelle-dr commented Nov 1, 2021

isabelle-dr left a comment

asvechnikov2 Nov 7, 2021

asvechnikov2 Nov 7, 2021

isabelle-dr commented Nov 16, 2021

barbeau Nov 16, 2021 •

edited

Loading

barbeau Nov 16, 2021 •

edited

Loading

lionel-nj Nov 16, 2021

github-actions bot commented Nov 16, 2021

barbeau left a comment

barbeau left a comment

asvechnikov2 left a comment

asvechnikov2 Nov 16, 2021

fix: False positives for StopTimeTimepointWithoutTimesNotice #1044

fix: False positives for StopTimeTimepointWithoutTimesNotice #1044

Conversation

lionel-nj commented Oct 26, 2021 • edited Loading

sample data 1 (no timepoint column)

sample data 2 (timepoint column + no empty values)

sample data 3 (timepoint with no times)

sample data 4 (approximate, with times)

sample data 5 (approximate, with no times)

maximearmstrong left a comment

Choose a reason for hiding this comment

maximearmstrong Oct 29, 2021

Choose a reason for hiding this comment

lionel-nj Nov 1, 2021

Choose a reason for hiding this comment

isabelle-dr commented Nov 1, 2021

lionel-nj commented Nov 1, 2021 • edited Loading

lionel-nj commented Nov 1, 2021 • edited Loading

isabelle-dr commented Nov 1, 2021

isabelle-dr left a comment

Choose a reason for hiding this comment

asvechnikov2 Nov 7, 2021

Choose a reason for hiding this comment

asvechnikov2 Nov 7, 2021

Choose a reason for hiding this comment

isabelle-dr commented Nov 16, 2021

barbeau Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

barbeau Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

lionel-nj Nov 16, 2021

Choose a reason for hiding this comment

github-actions bot commented Nov 16, 2021

barbeau left a comment

Choose a reason for hiding this comment

barbeau left a comment

Choose a reason for hiding this comment

asvechnikov2 left a comment

Choose a reason for hiding this comment

asvechnikov2 Nov 16, 2021

Choose a reason for hiding this comment

lionel-nj commented Oct 26, 2021 •

edited

Loading

lionel-nj commented Nov 1, 2021 •

edited

Loading

lionel-nj commented Nov 1, 2021 •

edited

Loading

barbeau Nov 16, 2021 •

edited

Loading

barbeau Nov 16, 2021 •

edited

Loading