Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clip negative FCI radiances #3013

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

gerritholl
Copy link
Member

@gerritholl gerritholl commented Dec 9, 2024

Optionally clip negative FCI radiances.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.10%. Comparing base (5984c29) to head (d404a79).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3013   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files         377      377           
  Lines       55163    55180   +17     
=======================================
+ Hits        53012    53029   +17     
  Misses       2151     2151           
Flag Coverage Δ
behaviourtests 3.94% <0.00%> (-0.01%) ⬇️
unittests 96.19% <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 Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Dec 9, 2024

Pull Request Test Coverage Report for Build 12391703039

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

  • 22 of 22 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 96.208%

Totals Coverage Status
Change from base Build 12227072168: 0.002%
Covered Lines: 53273
Relevant Lines: 55373

💛 - Coveralls

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

@gerritholl
Copy link
Member Author

rymdulf is lying, the tests were not successful. Failed with:

  Scenario Outline: Compare generated image with reference image -- @1.1         # features/image_comparison.feature:10
    Given I have a airmass reference image file from GOES17 resampled to <area>  # features/steps/image_comparison.py:54
[ WARN:[email protected]] global loadsave.cpp:241 findDecoder imread_('/app/ext_data/reference_images/reference_image_GOES17_airmass_<area>.png'): can't open/read file: check file path/integrity
    When I generate a new airmass image file from GOES17 with abi_l1b for <area> # features/steps/image_comparison.py:64
      Traceback (most recent call last):
        File "/app/venv/lib/python3.10/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
        File "/app/venv/lib/python3.10/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
        File "features/steps/image_comparison.py", line 82, in step_when_generate_image
          ls = scn.resample(area)
        File "/app/repository/satpy/scene.py", line 980, in resample
          self._resampled_scene(new_scn, destination, resampler=resampler,
        File "/app/repository/satpy/scene.py", line 869, in _resampled_scene
          destination_area = self._get_finalized_destination_area(destination_area, new_scn)
        File "/app/repository/satpy/scene.py", line 905, in _get_finalized_destination_area
          destination_area = get_area_def(destination_area)
        File "/app/repository/satpy/resample.py", line 231, in get_area_def
          return parse_area_file(get_area_file(), area_name)[0]
        File "/app/venv/lib/python3.10/site-packages/pyresample/area_config.py", line 147, in parse_area_file
          return _parse_yaml_area_file(area_file_name, *regions)
        File "/app/venv/lib/python3.10/site-packages/pyresample/area_config.py", line 201, in _parse_yaml_area_file
          raise AreaNotFound('Area "{0}" not found in file "{1}"'.format(area_name, area_file_name))
      pyresample.area_config.AreaNotFound: 'Area "<area>" not found in file "[\'/app/repository/satpy/etc/areas.yaml\']"'

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

Trying John Cintineos workaround.
Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

run behave test

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 13, 2024

The testing process was executed successfully. See the test results for this pull request here!

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

The testing process was executed successfully. See the test results for this pull request here!

Add a unit test for FCI radiance clipping.

The unit test fails, because there is no implementation yet.
Clip negative radiances when a keyword arguments asks for it.
Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

The testing process was executed successfully. See the test results for this pull request here!

@gerritholl gerritholl marked this pull request as ready for review December 16, 2024 17:38
Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

The testing process was executed successfully. See the test results for this pull request here!

@gerritholl
Copy link
Member Author

Good news:

  • Negative radiances are clipped.
  • No more black holes in the cloudtop RGB.

Cloudtop RGB

Bad news:

  • Still green holes in the night microphysics RGB.

Night microphysics RGB

Copy link
Member Author

@gerritholl gerritholl left a comment

Choose a reason for hiding this comment

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

start behave test

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

Starting to clone and test the repository pytroll/satpy

@rymdulf
Copy link

rymdulf commented Dec 16, 2024

The testing process was executed successfully. See the test results for this pull request here!

@gerritholl gerritholl requested a review from simonrp84 December 17, 2024 07:56
@gerritholl
Copy link
Member Author

It's clipping space pixels, which we probably don't want? How do ABI and AHI prevent that?

@gerritholl gerritholl marked this pull request as draft December 17, 2024 13:56
@gerritholl gerritholl marked this pull request as ready for review December 17, 2024 14:57
@djhoese
Copy link
Member

djhoese commented Dec 17, 2024

I think ABI, through normal ground processing, have masked space pixels. AHI has space pixels masked by default in Satpy I think (@simonrp84?) so maybe we just don't care? Or is radiance clipping not implemented for AHI?

@simonrp84
Copy link
Member

I don't understand your comment, @gerritholl: The space pixels are masked in the raw FCI data so it shouldn't be clipping them. They should already be nodata.

Also, @djhoese IIRc there's no clipping applied to the AHI data. It's the only modern GEO sat that doesn't mask space pixels in the data, but we do by default mask them in satpy.

@gerritholl
Copy link
Member Author

gerritholl commented Dec 18, 2024

I don't understand your comment, @gerritholl: The space pixels are masked in the raw FCI data so it shouldn't be clipping them. They should already be nodata.

Yes, but I had a bug that was not only setting negative radiances to a low radiance, but also nan radiances. It was retaining only pixels where data>lo was True, and this was false for the nan space pixels. Fixed that in d404a79.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

artefacts in FCI RGBs using 3.8 µm
5 participants