Skip to content

add raw counts calibration type to Metop-SG A1 reader (vii_l1b_nc.py)#3373

Open
tommyjasmin wants to merge 2 commits intopytroll:mainfrom
tommyjasmin:main
Open

add raw counts calibration type to Metop-SG A1 reader (vii_l1b_nc.py)#3373
tommyjasmin wants to merge 2 commits intopytroll:mainfrom
tommyjasmin:main

Conversation

@tommyjasmin
Copy link
Copy Markdown
Contributor

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 27.27273% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.33%. Comparing base (6c571f2) to head (2c057bb).
⚠️ Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
satpy/readers/vii_l1b_nc.py 27.27% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3373      +/-   ##
==========================================
- Coverage   96.35%   96.33%   -0.03%     
==========================================
  Files         466      466              
  Lines       59083    59093      +10     
==========================================
- Hits        56931    56925       -6     
- Misses       2152     2168      +16     
Flag Coverage Δ
behaviourtests 3.58% <0.00%> (-0.01%) ⬇️
unittests 96.42% <27.27%> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 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.

calibration:
counts:
standard_name: counts
units: "count"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 😁

Copy link
Copy Markdown
Member

@pnuu pnuu Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with Tommy on this, counts are quantized values of charge accumulated on the detector. Maybe via voltage, but anywho. Unit of "1" or "count" is fine by me, doesn't really matter other than we should be consistent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pnuu - yes, maybe joule is the correct units on the detector. Whatever it is, they cancel out. I admit a propensity for throwing out "volts" and "amps" too easily, for nostalgia's sake, trying to convince myself that the electrical engineering degree I got in the late 80s wasn't completely wasted time. Plus a joule is the energy to raise a 100 gram apple one meter, that is way more fun. 😂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit should definitely not "joule" or anything like that, as the counts are the output from the analogue-to-digital converter and are therefore a simple digital number rather than an SI-traceable unit.

I agree with @gerritholl. Ideally there should be no unit attached to counts at all. But if we do need a unit, can we say N/A for "not applicable" or something like that?

Copy link
Copy Markdown
Contributor Author

@tommyjasmin tommyjasmin Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies @simonrp84 - that was simply a poor attempt at humor, I am not in any way suggesting joule...
The point was that the energy reaching the sensor detector has units, the DN does not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

try:
self._integrated_solar_irradiance = self["data/calibration_data/band_averaged_solar_irradiance"].values
except KeyError:
self._integrated_solar_irradiance = self["data/calibration_data/Band_averaged_solar_irradiance"].values
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What files have the "B" and what files have the "b" naming?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djhoese - the test data I have, and am using for co-dev on the PyADDE server, have the cap-B:

(mtip) crud:MetOp tommyj$ ncdump -h W_XX-EUMETSAT-Darmstadt,SAT,SGA1-VII-1B-RAD_C_EUMT_20210510074835_G_D_20080223102004_20080223102104_T_B____.nc | grep Band
    	float Band_averaged_solar_irradiance(num_chan_solar) ;
    		Band_averaged_solar_irradiance:_FillValue = -999.f ;
    		string Band_averaged_solar_irradiance:long_name = "Band averaged solar irradiance" ;
    		string Band_averaged_solar_irradiance:units = "W/(m^2 µm)" ;
    		Band_averaged_solar_irradiance:valid_min = 0.f ;
    		Band_averaged_solar_irradiance:valid_max = 2400.f ;

I am unsure of the origin for this test data, but can find out if needed.

For some reason, back in 2024, the cap "B" was changed to a lowercase "b". See:

1c0ce37

I am under the assumption there must be test data with both variants in the wild, and added the try/except to support that.

Comment on lines +91 to +92
# xarray automatically applies scale_factor and add_offset when reading the netCDF.
# To get raw counts, reverse this process using the original parameters.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could ask xarray not to do this and then have the radiance calculation do the scaling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hhmmm... not sure what is most appropriate at the moment. Need to have a think on this one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty easy to turn off in ViiNCBaseFileHandler:
super().__init__(filename, filename_info, filetype_info, auto_maskandscale=True)

Turning off auto_maskandscale does mean that a few other netCDF variables need to be manually unpacked. Neither Tommy or I were sure which approach would be preferable for Satpy. We ended up opting for the least invasive approach for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not completely against the way it is implemented, but others may feel differently.

@djhoese djhoese added enhancement code enhancements, features, improvements component:readers labels Apr 10, 2026
@djhoese djhoese self-assigned this Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:readers enhancement code enhancements, features, improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants