fix(checks): hoist camera threshold validations to fail early on came… - #459
fix(checks): hoist camera threshold validations to fail early on came…#459Chama2001 wants to merge 14 commits into
Conversation
|
👋 Hi @Chama2001 — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
kstonekuan
left a comment
There was a problem hiding this comment.
Welcome, and thank you for sticking with this after the week you have had on my account.
The substance is right, which is the hard part. The guards are at the top of both functions, ahead of selected_cameras, so they fire on a camera-less episode, and each message names the parameter the way the signature spells it rather than the internal settings field. That is exactly what #447 asked for. Gate is clean.
Four things before it can go in.
Nothing tests any of it. The diff is source only, and none of the five messages appears anywhere in the repository:
0 files: "black_pixel_threshold must be"
0 files: "black_frame_amount_pct must be"
0 files: "freeze_min_duration_s must be"
0 files: "freeze_noise_db must be"
0 files: "bright_luma_threshold must be"
This is the one I cannot wave through, because it is the same shape as the bug you are fixing: a guard that exists and nothing holds. Remove any line you added and the suite stays green. #447's definition of done asks for every case in its table, with at least one running against a camera-less episode, since a test using a video episode passes today and proves nothing.
Three of the five parameters still accept True. Measured on your branch, camera-less episode:
frame_stats black_pixel_threshold TypeError: black_pixel_threshold must be an integer
frame_stats black_frame_amount_pct TypeError: black_frame_amount_pct must be an integer
frame_stats freeze_min_duration_s ACCEPTED (True read as 1)
frame_stats freeze_noise_db ACCEPTED (True read as 1)
frame_stats bright_luma_threshold ACCEPTED (True read as 1)
signal_quality black_pixel_threshold TypeError: black_pixel_threshold must be an integer
signal_quality freeze_min_duration_s ACCEPTED (True read as 1)
signal_quality freeze_noise_db ACCEPTED (True read as 1)
bool subclasses int, so True sails through any range comparison as 1. You caught it on the two int-typed parameters and not on the three float-typed ones, and the float ones need it just as much.
Two idioms that diverge from the rest of the file, both worth matching rather than arguing about:
type(x) is bool works, but everywhere else in this repo the check is isinstance(x, bool). See _field_guards.require_float, FrameSampling.__post_init__, and the two guards #441 and #446 just landed in this same function's neighbour.
TypeError for the bool case is a new convention here. require_int raises ValueError for exactly this ("must be an int, got bool"), and #441 and #446 raise ValueError. Callers writing except ValueError around a check would miss yours.
Also import math inside two function bodies: checks.py imports numpy at module level and #441/#446 used np.isfinite. Use that and drop both local imports.
One thing the issue asked you to rule on either way. It flagged that camera_fps_conformance's max_plausible_fps and downsample_tolerance_fps look like the same shape and that I had not verified them. Including them is fine and excluding them is fine; going quiet on them is the thing to avoid. Say which and why.
None of this is a rethink. It is one test file, three more bool clauses, and two find-and-replaces. Ping me when you push and I will look straight away.
Closes #447
Summary
Moved threshold guards before the camera loop in
camera_frame_statsandcamera_signal_quality.Why
As discussed in #447, previously these guards were inside the
for topic in selected_cameras:loop, meaning they were skipped entirely on camera-less episodes. This change hoists them to the top of the functions so they fail early and correctly.Validation
Applied the exact pattern requested in the issue description.