Add raw "counts" calibration type to "metimage_l1b_nc" reader by tommyjasmin · Pull Request #3373 · pytroll/satpy

tommyjasmin · 2026-04-09T20:32:04Z

Some SSEC applications depend on raw counts from the sensor, which several satpy readers already support. This adds a counts calibration to the METimage (VII) L1B reader, benefiting the SSEC PyADDE project.

Closes #xxxx
Tests added
Fully documented
Add your name to AUTHORS.md if not there already

codecov · 2026-04-10T00:21:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.38%. Comparing base (1e68f2a) to head (59b1296).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3373      +/-   ##
==========================================
+ Coverage   96.36%   96.38%   +0.01%     
==========================================
  Files         466      466              
  Lines       59699    59757      +58     
==========================================
+ Hits        57530    57596      +66     
+ Misses       2169     2161       -8

Flag	Coverage Δ
behaviourtests	`3.55% <0.00%> (-0.01%)`	⬇️
unittests	`96.47% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

djhoese · 2026-04-10T14:09:04Z

    calibration:
+      counts:
+        standard_name: counts
+        units: "count"


It looks like this is inconsistent in Satpy, but I think counts should have units of "1". @simonrp84 @mraspaud @pnuu @gerritholl thoughts? Even the custom reader documentation says to do units: "count" as done here.

In the terminology of ISO 80000-1:2013 (the version I have a copy of, but I doubt it has radically changed), counts would be a quantity of dimension one (also referred to as dimensionless for historical reasons, but this is not strictly correct). ISO 80000-1 defines a unit of measurement as:

real scalar quantity, defined and adopted by convention, with which any other quantity of the same kind can
be compared to express the ratio of the second quantity to the first one as a number

Thinking about it, I don't think "count" meets this definition. Neither does 1. There is no quantity measured by "200 counts" that is 100 counts more than "100 counts". There is no convention that defines what 1 count is. And 1 is just a number.

Counts are recognised by UDUNITS, but that package has a rather liberal approach including anything that people use, and is not prescriptive on what is correct.

We can be pragmatic and use count, even if it is not strictly a unit.

We can be pragmatic and use 1, like we do for other quantities that have no unit, for example:

satpy/satpy/etc/readers/viirs_l2.yaml

Lines 110 to 113 in c1537e3

Angstrom_Exponent_Land_Ocean_Best_Estimate:

name: Angstrom_Exponent_Land_Ocean_Best_Estimate

long_name: Deep Blue/SOAR Angstrom exponent over land and ocean

units: "1"

Or we can be strict, and in that case I would set units empty or leave it out entirely.

When there is no unit "1" is not the worst to use, because ISO 80000-1 says we multiply a number with its unit to get a quantity, and multiplication with 1 is a no-op. But it doesn't really work, because counts — digital number — does not really express a quantity of anything in the first place.

BS EN ISO 80000-1-2013--[2017-03-23--10-09-20 AM].pdf

In conclusion, I think counts — digital number — should not have a unit at all, because it is not calibrated and does not express a quantity.

I think in other parts of Satpy we (or more likely past Dave's code) assume a non-empty units or at least a not-None units value.

@gerritholl - thank you for the commentary. I would push back on one thing, however.

You say a digital number does not express a quantity, based on ISO-80000-1, but I would argue it does.

A DN compares an incoming analog signal with a known reference, and scales that ratio. That's a quantity.

In fact I would even say DN is itself could be considered an appropriate answer here. But what is a DN in this world - it is a quantity where the physical units, volts/volts, cancel out with a result of 1.

I think the answer comes down to whether we prefer strict ISO compliance, or human readability. Counts is fairly well understood and prevents confusion with calibrated values.

One of y'all should just make an executive decision. 😁

Of course the counts relate to a physical quantity, or the detector output couldn't be used to measure anything physical. But the number refers to a digital detector output, without a universal definition. The meaning of the difference between "1 count" and "2 counts", or even "-10 counts" (HIRS) depends on the instrument/detector. That is clearly not the counts I learned when covering CCDs in university, which are the number of photons falling onto the pixel multiplied by the quantum efficiency, and thus cannot be negative. That might just be a scaling for digital storage reasons, but it shows there is no one way to define a count. Microwave radiometers are again different entirely.

There's nothing about count in ISO 80000-1 explicitly. A web search for ISO "count" in a broader context yields mostly results about particle count in a context of ionising radiation (and even kilocount), something entirely different with the same word, and something with a direct physical definition. Those "counts" have unit 1: The unit of Becquerel is s⁻¹ (not count·s⁻¹).

If we must have a unit, I would argue for 1, because I don't think there is such a thing as a unit "count".

Ok, let's go with "1". If I don't hear any further reasons for something else, I'll update the PR tomorrow, thanks everyone!

If y'all want, once this PR is resolved, I'm happy to make a separate PR to update any remaining raw counts calibration references that are inconsistent.

Yeah I think it'd be nice to have an issue, rather than PR, where we can discuss what the correct unit for this should be.

Yeah I think it'd be nice to have an issue, rather than PR, where we can discuss what the correct unit for this should be.

@simonrp84 - agree but to be clear, separate GitHub ticket/issue for adopting a convention system-wide. I still want to get this PR pushed through independently.

# Conflicts: # satpy/etc/readers/metimage_l1b_nc.yaml

- counts units: "count" -> "1" per review consensus (all 20 channels) - restore _FillValue at masked pixels instead of NaN->astype (undefined behavior that yielded 0), and round before the integer cast to guard against float truncation - clarify the capital-B solar irradiance fallback: operational products use lowercase, pre-launch test granules used capital B - add unit tests: counts round-trip of stored integers, fill restoration, unscaled-variable counts, capital-B fallback Verified counts bit-identical to on-disk digital numbers on a real SGA1-VII-1B-RAD operational granule (solar and thermal channels). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EorkxydjrSncteX8QtR477

tommyjasmin · 2026-09-16T01:06:20Z

Thanks for the patience on this one — it's updated and ready for another look:

Merged with main (the reader is now metimage_l1b_nc post-reorg; changes carried across).
Counts units switched to "1" on all 20 channels, per the discussion above.
@djhoese on the capital-B question: checked our granules — operational products use lowercase band_averaged_solar_irradiance; the capital-B form is in pre-launch test data (2021-era _T_ dissemination granules). The fallback stays, now with a comment saying exactly that.
Found and fixed a real bug while adding tests: _FillValue pixels (masked to NaN by xarray) went through astype(uint16) — undefined behavior that happened to yield 0. Counts now restore the original fill value at masked pixels and round before the integer cast. Verified bit-identical to the on-disk digital numbers on a real SGA1 VII L1B operational granule, solar and thermal channels, fill included.
Unit tests cover the counts round-trip, fill restoration, the unscaled-variable path, and the capital-B fallback.

# Conflicts: # satpy/readers/metimage_l1b_nc.py # satpy/tests/reader_tests/test_metimage_l1b_nc.py

- Counts calibration moved into _calibrate_counts; the restored fill value is now also set as the "_FillValue" attribute (cast to the stored dtype). - Fall back to missing_value when a variable has no _FillValue, so pixels xarray masked via missing_value are restored instead of NaN reaching astype. - Keep valid_min/valid_max for the counts calibration only: they describe the packed on-disk integers, so they are correct for counts and still removed for radiance/reflectance/brightness temperature. - Combine the counts tests with pytest.mark.parametrize and cover the missing_value path and the valid range handling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CodeScene flagged _create_l1b_file as a Large Method after the counts variables were added inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

tommyjasmin · 2026-10-01T17:57:30Z

Hey @djhoese - I don't think this single test failure is a problem on my end, I think y'all have to work that one since it's failing at import before the test even runs.

pnuu · 2026-10-01T18:10:42Z

That's the unstable build. I restarted the build since I didn't even get the logs from that one. But yeah, the unstable build is something that typically is something to be handled outside the triggering PR, it's just a canary so that we see things getting broken with bleeding-edge versions of dependencies.

coveralls · 2026-10-01T18:18:25Z

Coverage Report for CI Build 36783476330

Coverage increased (+0.005%) to 96.45%

Details

Coverage increased (+0.005%) from the base build.
Patch coverage: No coverable lines changed in this PR.
4 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

4 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
readers/core/metimage_nc.py	2	96.86%
readers/metimage_l1b_nc.py	2	97.33%

Coverage Stats


Relevant Lines:	59602
Covered Lines:	57486
Line Coverage:	96.45%
Coverage Strength:	2.89 hits per line

💛 - Coveralls

pnuu · 2026-10-01T18:29:12Z

Still nothing. Errors out already in Set up job stage.

djhoese · 2026-10-02T17:30:07Z

Regarding failing CI I think the new shapely is just not compatible with other libraries we're installing from conda-forge. We may have to wait a bit for it to be fixed upstream or remove shapely from the unstable install or install shapely from the nightly wheels:

https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/shapely/

No METimage product uses missing_value (all channels define _FillValue), and xarray only masks to NaN when a fill attribute exists, so the fallback guarded nothing. Store _FillValue in .attrs as a builtin int, following the Satpy preference for builtin types in attributes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

All METimage channels are stored as scaled integers with scale_factor, add_offset and _FillValue, so drop the fallback defaults and the test of an unscaled variable without a fill value, which no real file has. A file missing these attributes now raises instead of returning wrong counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

djhoese

Looks good to me. Any other last reviews?

ameraner

LGTM too, thanks for this!

add raw counts calibration type

1fac26b

tommyjasmin requested review from djhoese and mraspaud as code owners April 9, 2026 20:32

whoops. messed up indenting, and should catch KeyError

2c057bb

djhoese reviewed Apr 10, 2026

View reviewed changes

Comment thread satpy/readers/metimage_l1b_nc.py

djhoese reviewed Apr 10, 2026

View reviewed changes

Comment thread satpy/readers/vii_l1b_nc.py Outdated

djhoese added enhancement code enhancements, features, improvements component:readers labels Apr 10, 2026

djhoese self-assigned this Apr 10, 2026

tommyjasmin and others added 2 commits September 15, 2026 19:39

Merge remote-tracking branch 'upstream/main'

f6e4cc5

# Conflicts: # satpy/etc/readers/metimage_l1b_nc.yaml

djhoese reviewed Sep 16, 2026

View reviewed changes

Comment thread satpy/tests/reader_tests/test_metimage_l1b_nc.py

djhoese reviewed Sep 16, 2026

View reviewed changes

Comment thread satpy/readers/metimage_l1b_nc.py Outdated

djhoese reviewed Sep 16, 2026

View reviewed changes

Comment thread satpy/readers/metimage_l1b_nc.py Outdated

tommyjasmin and others added 3 commits September 30, 2026 15:35

Merge remote-tracking branch 'upstream/main'

4d47eb2

# Conflicts: # satpy/readers/metimage_l1b_nc.py # satpy/tests/reader_tests/test_metimage_l1b_nc.py

Move counts test variables into a helper to keep _create_l1b_file small

df7c2c4

CodeScene flagged _create_l1b_file as a Large Method after the counts variables were added inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

djhoese reviewed Oct 2, 2026

View reviewed changes

Comment thread satpy/readers/metimage_l1b_nc.py Outdated

djhoese reviewed Oct 2, 2026

View reviewed changes

Comment thread satpy/tests/reader_tests/test_metimage_l1b_nc.py Outdated

djhoese reviewed Oct 2, 2026

View reviewed changes

Comment thread satpy/tests/reader_tests/test_metimage_l1b_nc.py Outdated

djhoese reviewed Oct 2, 2026

View reviewed changes

Comment thread satpy/tests/reader_tests/test_metimage_l1b_nc.py Outdated

djhoese approved these changes Oct 6, 2026

View reviewed changes

ameraner approved these changes Oct 6, 2026

View reviewed changes

djhoese changed the title ~~add raw counts calibration type to Metop-SG A1 reader (vii_l1b_nc.py)~~ Add raw "counts" calibration type to "metimage_l1b_nc" reader Oct 6, 2026

djhoese merged commit 3de8529 into pytroll:main Oct 6, 2026
18 checks passed

	Angstrom_Exponent_Land_Ocean_Best_Estimate:
	name: Angstrom_Exponent_Land_Ocean_Best_Estimate
	long_name: Deep Blue/SOAR Angstrom exponent over land and ocean
	units: "1"

Conversation

tommyjasmin commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommyjasmin Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tommyjasmin Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tommyjasmin commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tommyjasmin commented Oct 1, 2026

Uh oh!

pnuu commented Oct 1, 2026

Uh oh!

coveralls commented Oct 1, 2026

Coverage Report for CI Build 36783476330

Coverage increased (+0.005%) to 96.45%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

pnuu commented Oct 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djhoese commented Oct 2, 2026

Uh oh!

Uh oh!

djhoese left a comment

Choose a reason for hiding this comment

Uh oh!

ameraner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tommyjasmin commented Apr 9, 2026 •

edited

Loading

codecov Bot commented Apr 10, 2026 •

edited

Loading

tommyjasmin Apr 14, 2026 •

edited

Loading

tommyjasmin Apr 14, 2026 •

edited

Loading