[uss_qualifier] netrid: cover net0260 timestamp accuracy #822
Conversation
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
3 times, most recently
from
7e80297 to
21e4c5a
Compare
| @@ -914,6 +925,44 @@ def _evaluate_normal_sp_observation( | |||
| details=f"{mapping.injected_flight.uss_participant_id}'s flight with injection ID {mapping.injected_flight.flight.injection_id} in test {mapping.injected_flight.test_id} had telemetry index {mapping.telemetry_index} at {injected_telemetry.timestamp} with lat={injected_telemetry.position.lat}, lng={injected_telemetry.position.lng}, alt={injected_telemetry.position.alt}, but Service Provider reported lat={observed_position.lat}, lng={observed_position.lng}, alt={observed_position.alt} at {mapping.observed_flight.query.query.request.initiated_at}", | |||
| ) | |||
|
|
|||
| # Due to how implicit dicts are deserialized and the timestamp_accuracy is specified in the OpenAPI file, we need to look into | |||
There was a problem hiding this comment.
Do note that for v19 there is no default. I don't think this change anything in implementation though given that we mainly target v22a, but maybe just update the comment to reflect that.
|
|
||
| #### ⚠️ Service Provider timestamp accuracy is correct check | ||
|
|
||
| **[astm.f3411.v22a.NET0260,Table1,5](../../../../requirements/astm/f3411/v22a.md)** requires that relevant Remote ID data, consistent with the common data dictionary, be reported by the Service Provider. Injected flight data had known timestamp accuracy, but the accuracy reported by the Service Provider did not match it. |
There was a problem hiding this comment.
Could you be more specific as to what would fail that check?
In addition I see that the implementation does a strict equality check, I don't think that's reasonable.
First because it is a float value so at least it should be an interval check.
Then, I don't think it would necessary be a failing criteria for a SP to report an accuracy different than what is injected. E.g. if this accuracy is implemented as buckets by the SP.
The way I interpret it, given that the accuracy is meant to reflect an uncertainty bound, reporting more uncertainty than the reality is not wrong, however reporting less uncertainty than reality is wrong.
There was a problem hiding this comment.
Regarding the wording: I've been reusing wording for similar checks in the same file to remain consistent. I can make the end of the sentence more compact, but if we are not happy with the overall wording I'd change the whole file in a separate PR
There was a problem hiding this comment.
Regarding the exact check for the accuracy: indeed, we should probably allow a little buffer for float comparison. However the spec is quite precise as to what precision.
About checking for bounds, I disagree: the spec states
[...] Expressed in seconds, precise to 1/10ths of seconds. The accuracy reflects the 95% uncertainty bound value for the timestamp.
Changing the bound would mean that the value does not reflect the 95% uncertainty bound anymore: here I see no reason why an SP should be allowed to change the value by a tenth of a second or more.
There was a problem hiding this comment.
@mickmis given similar discussions we've had at the weekly meeting regarding that we would generally expect to read back what we have injected, I'd say the current approach is fine?
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
from
21e4c5a to
a0909d8
Compare
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
2 times, most recently
from
1c6a230 to
3f8a6a2
Compare
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
from
3f8a6a2 to
ca24b21
Compare
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
3 times, most recently
from
7c6d042 to
c89472f
Compare
|
|
||
| #### ⚠️ Service Provider timestamp accuracy is correct check | ||
|
|
||
| **[astm.f3411.v22a.NET0260,Table1,5](../../../../requirements/astm/f3411/v22a.md)** requires that relevant Remote ID data, consistent with the common data dictionary, be reported by the Service Provider. The observed timestamp accuracy differs from the injected one. |
There was a problem hiding this comment.
Wrong requirement:
| **[astm.f3411.v22a.NET0260,Table1,5](../../../../requirements/astm/f3411/v22a.md)** requires that relevant Remote ID data, consistent with the common data dictionary, be reported by the Service Provider. The observed timestamp accuracy differs from the injected one. | |
| **[astm.f3411.v22a.NET0260,Table1,6](../../../../requirements/astm/f3411/v22a.md)** requires that relevant Remote ID data, consistent with the common data dictionary, be reported by the Service Provider. The observed timestamp accuracy differs from the injected one. |
| details=f"Timestamp accuracy not present in Service Provider {mapping.injected_flight.uss_participant_id}'s response for flight with injection ID {mapping.injected_flight.flight.injection_id} in test {mapping.injected_flight.test_id} with telemetry index {mapping.telemetry_index}", | ||
| ) | ||
|
|
||
| if "timestamp_accuracy" in raw_state: |
There was a problem hiding this comment.
nit: put in else of above?
There was a problem hiding this comment.
That would be Ok, but I'm not a huge fan of nesting checks this way?
The current approach means the check for "is the field there" completes before we register/start the check for comparing the values.
If the comparison check is nested within, then the "is the field there" check completes after the comparison check, which feels weird here (although, in this case it should cause no problem).
So far I've always been following the approach of "one check open at a time unless otherwise required" but I'm happy to change that.
The above and the fact that another level of nesting does not improve the readability (in my opinion) would nudge me on the side of leaving it as-is?
There was a problem hiding this comment.
Yeah you are right actually 👍
| @@ -49,6 +49,7 @@ def _rect_str(rect) -> str: | |||
|
|
|||
| VERTICAL_SPEED_PRECISION = 0.1 | |||
| SPEED_PRECISION = 0.05 | |||
| TIMESTAMP_ACCURACY_EPSILON = 0.05 | |||
There was a problem hiding this comment.
nit: Suffix with _PRECISION for consistency?
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
from
c89472f to
4bb50aa
Compare
Shastick
force-pushed
the
net-0260-timestamp-accuracy
branch
from
4bb50aa to
5b7af59
Compare
Expand the generic
NominalBehaviorscenario to check:timestamp_accuracyfieldBecause of the way implicit dicts works, checking for the presence of this field requires to look at the raw response Json (see comment in the code)
Part of #754