Fix inert yapc.score photon filtering and fail loudly on unsupported YAPC configurations (fixes #648) - #649
Open
dshean wants to merge 1 commit into
Open
Fix inert yapc.score photon filtering and fail loudly on unsupported YAPC configurations (fixes #648)#649dshean wants to merge 1 commit into
dshean wants to merge 1 commit into
Conversation
…ail loudly The yapc score photon-selection threshold was silently inert: the filter comparisons in Atl03DataFrame (atl03x) and Atl03Reader (legacy) compared the photon score against the FieldElement object rather than its value, which resolved through FieldElement's implicit operator bool, so any non-zero score behaved as a threshold of 1 and the parameter had no effect. The comparisons now use the parameter value directly. Also made previously silent failure modes fail loudly: - yapc versions outside 0-3 are rejected when the parameters are parsed - yapc versions 1-3 on atl03x (not implemented there) now raise an alert instead of silently returning all-zero scores - yapc version 0 against pre-release-006 granules (no weight_ph dataset) now raises an alert on both atl03x and the legacy reader instead of silently returning zero scores (which, with the fixed comparison, would have silently filtered out every photon) Documented the score scale: scores read from release 007 granules are the granule weight_ph values on the 0-65535 saturation-normalized scale; release 006 granule scores and server-computed (version 1-3) scores are 0-255. The threshold is compared against the raw values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Root cause
The photon-selection threshold documented for
yapc.scorehas beensilently inert on both atl03x and the legacy reader (#648). The filter
code exists and looks correct:
but
scoreis aFieldElement<uint16_t>, andFieldElementdefinesoperator bool() const { return value != 0; }and nooperator T().Overload resolution therefore converts the field object to
bool,and the comparison is against 0 or 1 — never the requested threshold.
Any non-zero
scorebehaves as "score >= 1", which explains everysymptom reported in #648:
{version: 0, score: N}: identical results for N = 0/150/1000/3300/8000, minimum returned
yapc_score= 1 (weight-0 photonsare typically already excluded by the default
quality_phfilter);{version: 3, score: N}: selection changes once whenany N > 0 is supplied (score-0 photons dropped) and is then
insensitive to the value of N.
Additionally,
{version: 1..3}on atl03x silently returned all-zeroscores (no v1-3 implementation exists in the dataframe path and nothing
rejected the request), and
{version: 0}against pre-R006 granules(no
weight_phdataset) silently returned zero scores on both paths.With the comparison fixed, that last case would have become worse —
a non-zero threshold against all-zero scores silently returns an empty
result — so it now errors instead.
What changed
Atl03DataFrame.cpp/Atl03Reader.cpp: the five comparison sitesuse
parms.yapc.score.value(the same pattern the neighboring ATL24confidence filter already uses).
Atl03DataFrame.cpp(atl03x): requests withyapc.version != 0andrequests with
version == 0against pre-R006 granules now throw,which surfaces as a CRITICAL alert on the response stream instead of
silent zero scores.
Atl03Reader.cpp(legacy atl03s/sp, atl06/p):YapcScorenow throwsfor
version == 0against pre-R006 granules (v1-3 already threw forinvalid versions).
Atl03Parameters.cpp:yapc.versionoutside 0-3 is rejected atparameter parse time.
docs/rtd/source/user_guide/icesat2.md: documents the score scale(below) and the loud rejection of v1-3 on atl03x.
Semantics decision: v0 score scale
With
version: 0the scores are the granuleweight_phvalues readverbatim, and the threshold is compared against those raw values:
0-65535 for R007 granules (DDA-03 saturation-normalized weights;
65535 = saturation density), 0-255 for R006. Server-computed v1-3
scores remain 0-255. I kept the raw-scale comparison rather than
normalizing because (a) the returned
yapc_scorecolumn is the rawvalue, so threshold and column stay directly comparable, (b) any
rescaling would silently change meaning again — the failure mode this
PR removes — and (c) the R007 scale is the product's own documented
scale (ATL03 ATBD R007 §5.2). The docs previously stated 0-255
unconditionally; they now state the scale per source. (Happy to switch
to a normalized comparison if you'd rather keep the documented 0-255
interface stable across releases — it's a two-line change on top of
this.)
Tests
selftests/parameters.lua(offline): invalidyapc.versionrejectedat parse.
selftests/atl03_dataframe.lua(cloud): score threshold monotonicallyreduces row counts and enforces the minimum returned score on an R007
granule already used by the repo's selftests
(
ATL03_20200304065203_10470605_007_01.h5);{version: 3}on atl03xreturns an empty dataframe with an alert. Assertions are relational
(monotonic counts, min-score bounds) rather than magic row counts, so
they are robust to granule reprocessing. No existing expected values
were touched.
clients/python/tests/test_atl03x.py(live endpoint): the same twobehaviors through the Python client, against the R007 twin of the
existing test granule (
ATL03_20181019065445_03150111_007_01.h5).Testing performed vs. deferred to CI
Honest status: I could not build the server locally (macOS box without
the buildenv container). What I did run:
clang++ -std=c++20 -fsyntax-onlyover every modified C++ file plusIcesat2Parameters.cpp,Atl06DispatchParameters.cpp, andSurfaceFitter.cppas consumers, against the repo headers (Linuxtimer_t/UUID_STR_LEN/BUILDINFOshimmed) — clean;luac -pon both modified selftests;python -m py_compileandcodespell (repo pre-commit config) on the Python test and docs.
Not run:
make selftest(needs a server build; the newparameters.luacase is offline and will run there), the cloudselftest, and the Python client tests (need a deployed build with this
change). I'd rely on your testrunner for those; the failure signatures
in #648 were verified empirically against the public v5.5.0 cluster,
and the new tests assert exactly the behaviors that were observed
broken.
Possible follow-up (not in this PR)
Making
FieldElement::operator bool()explicitwould have turned allfive silent-conversion sites into compile errors and prevents
recurrence; I left it out to keep this diff minimal since I could not
compile the full tree to chase the fallout. Can open a separate issue
if useful.
Fixes #648
🤖 Generated with Claude Code