Skip to content

Conversation

@cesclc
Copy link

@cesclc cesclc commented Oct 28, 2025

Updates the VII reader stack so recent changes are fully covered and exercised. The reader now leaves the solar-zenith correction to the compositor (i.e., correction was previously performed twice), tolerates multiple filename layouts for L1B NetCDF products, and parses a wider range of sensing start/end timestamps attributes inside NetCDF. Adds the new VII daytime-only and night-time composites along with their YAML wiring, refreshes the reader unit tests to match the calibration and timestamp updates.

@ameraner
Copy link
Member

pre-commit.ci autofix

Copy link
Member

@ameraner ameraner left a comment

Choose a reason for hiding this comment

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

Hi Francesc, thanks for this! I left some comments inline.

@ameraner ameraner added bug enhancement code enhancements, features, improvements component:readers labels Oct 28, 2025
@howff
Copy link

howff commented Nov 6, 2025

When I test this with VII_L1B_CrossRef_TDP_V2 it complains about missing key data/calibration_data/band_averaged_solar_irradiance. I can comment that out as a workaround...

However it always crashes with a Segmentation Fault when I try to write an image. Maybe this isn't a fault of the reader, I don't know, but it doesn't crash with any other data. I've tried these tricks and it still crashes:

ulimit -n 4096
export DASK_NUM_WORKERS=1
export OMP_NUM_THREADS=1

@ameraner
Copy link
Member

As discussed, we should also apply the name change from vii_* to metimage_* (in this PR or another one). We should also update the names here then https://github.com/pytroll/satpy/blob/main/doc/source/examples/vii_l1b_nc.rst.

@ameraner
Copy link
Member

When I test this with VII_L1B_CrossRef_TDP_V2 it complains about missing key data/calibration_data/band_averaged_solar_irradiance. I can comment that out as a workaround...

However it always crashes with a Segmentation Fault when I try to write an image. Maybe this isn't a fault of the reader, I don't know, but it doesn't crash with any other data. I've tried these tricks and it still crashes:

ulimit -n 4096
export DASK_NUM_WORKERS=1
export OMP_NUM_THREADS=1

Hi @howff , the KeyError is an incompatibility with the currently public test data. We are now adapting the reader so that it will work with real data.
The NetCDF issue is a tricky one - let's keep that discussion in the other issue that you found already.

Copy link
Member

@ameraner ameraner left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Some more lines need to be removed and a small typo, I can commit this now myself.

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.33%. Comparing base (140a289) to head (995ba53).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
satpy/readers/core/vii_nc.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3277   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files         463      463           
  Lines       58879    58889   +10     
=======================================
+ Hits        56720    56731   +11     
+ Misses       2159     2158    -1     
Flag Coverage Δ
behaviourtests 3.60% <0.00%> (-0.01%) ⬇️
unittests 96.42% <96.77%> (+<0.01%) ⬆️

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.

@ameraner
Copy link
Member

ameraner commented Dec 2, 2025

Codecov is marking that we're not testing the ValueError for unrecognised timestamps - that should be an easy addition, could you maybe quickly add it?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 19863014968

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on main at 96.415%

Totals Coverage Status
Change from base Build 19840971577: 96.4%
Covered Lines: 56611
Relevant Lines: 58716

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug component:readers enhancement code enhancements, features, improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants