Skip to content

Modified the issue with the calibration coefficient indices for FY-3 satellite data reader#2959

Merged
mraspaud merged 4 commits intopytroll:mainfrom
sgxl:main
Nov 20, 2024
Merged

Modified the issue with the calibration coefficient indices for FY-3 satellite data reader#2959
mraspaud merged 4 commits intopytroll:mainfrom
sgxl:main

Conversation

@sgxl
Copy link
Contributor

@sgxl sgxl commented Oct 31, 2024

"There were issues with etc/readers/mersi2_l1b.yaml and etc/readers/mersi3_l1b.yaml. Minor code changes have been made:

According to the MERSI data document, the 'calibration_index' in Satpy was incorrect. The VIS_Cal_Coeff dataset includes calibration coefficients for both 250m (Data/EV_250_Aggr.1KM_RefSB) and 1km (Data/EV_1KM_RefSB) bands. The calibration coefficients are ordered by band sequence. Previously, the calibration indices for Data/EV_1KM_RefSB started from 0, which is incorrect; they should start after the last index of the 250m visible band.

image

Xuanhan Lai

sgxl added 3 commits October 31, 2024 11:25
Fixed bugs related to incorrect "calibration_index" values for each visible band (from band 5 to band 19) with 1 km spatial resolution. 

Xuanhan Lai, 2024-10-31
Fixed bugs related to incorrect "calibration_index" values for each visible band (from band 5 to band 19) with 1 km spatial resolution. 

Xuanhan Lai, 2024-10-31
@sgxl sgxl requested review from djhoese and mraspaud as code owners October 31, 2024 03:49
@mraspaud mraspaud requested a review from simonrp84 October 31, 2024 07:19
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Thanks!

I think this makes sense to me so I'm OK with the index changes, but why the removal of the "radiance" calibration?

Also, do you have an example of what one of the visible channels looked like before and after these changes? Is there an obvious improvement?

@djhoese
Copy link
Member

djhoese commented Oct 31, 2024

Are these indexes still valid/correct when the other coefficient list is used:

coeffs = self["/attr/VIR_Cal_Coeff"]

"/attr/VIR_Cal_Coeff"

Fix bugs related to the incorrect removal of radiance calculations.
@sgxl
Copy link
Contributor Author

sgxl commented Nov 1, 2024

Thank you for pointing out the mistakes in my code regarding the incorrect removal of 'radiance.' These issues may have been due to using older versions of the code. I have fixed them.

I have plotted the comparison charts before and after the "calibration_index" modifications:
image

@sgxl
Copy link
Contributor Author

sgxl commented Nov 1, 2024

I have already checked the fy3a_mersi1_l1b.yaml, fy3b_mersi1_l1b.yaml, and fy3a_mersi1_l1b.yaml files before the pull request, and I did not find any obvious errors with the calibration index.

@sgxl
Copy link
Contributor Author

sgxl commented Nov 1, 2024

https://essd.copernicus.org/articles/12/1123/2020/essd-12-1123-2020-f04-web.png

Thank you for pointing out the mistakes in my code regarding the incorrect removal of 'radiance.' These issues may have been due to using older versions of the code. I have fixed them.

I have plotted the comparison charts before and after the "calibration_index" modifications: image

And for ocean water spectra, there typically should not be an absorption dip at 443 nm. The common shape of water body spectra can be referred to: https://essd.copernicus.org/articles/12/1123/2020/essd-12-1123-2020-f04-web.png

@codecov
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.08%. Comparing base (d6b8d01) to head (927769f).
Report is 308 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2959      +/-   ##
==========================================
- Coverage   96.09%   96.08%   -0.02%     
==========================================
  Files         377      377              
  Lines       55125    55125              
==========================================
- Hits        52975    52967       -8     
- Misses       2150     2158       +8     
Flag Coverage Δ
behaviourtests 3.94% <ø> (ø)
unittests 96.18% <ø> (-0.02%) ⬇️

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.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11622341982

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 96.191%

Files with Coverage Reduction New Missed Lines %
satpy/tests/utils.py 2 93.16%
satpy/tests/reader_tests/gms/test_gms5_vissr_l1b.py 3 98.67%
satpy/tests/reader_tests/gms/test_gms5_vissr_navigation.py 3 97.18%
Totals Coverage Status
Change from base Build 11519685240: -0.02%
Covered Lines: 53211
Relevant Lines: 55318

💛 - Coveralls

@mraspaud
Copy link
Member

mraspaud commented Nov 4, 2024

@sgxl thanks for your work! We are just waiting for @simonrp84 to review this PR as he has been working with the FY3 data and readers...

@simonrp84
Copy link
Member

Reminder to myself to look at this early next week!

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Approving this as I got the green light from @simonrp84 !

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants