Add support to Pyspectral NIRReflectance masking limit #1390

pnuu · 2020-10-05T08:28:26Z

This is adds support to masking_limit keyword argument for NIRReflectance modifier added in pytroll/pyspectral#113

Tests added
Passes flake8 satpy
Fully documented

I'm not sure where to document this feature, but the usage shown below.

This disables the masking altogether. See pytroll/pyspectral#113 for an example image. This is the old default behaviour, which was changed in Pyspectral May (pytroll/pyspectral@e9a234f#diff-2a5232e6ece84bcd6cc7c97d82a7a574).

  nir_reflectance_no_masking:
    compositor: !!python/name:satpy.composites.NIRReflectance
    prerequisites:
      - name: IR_108
    optional_prerequisites:
    - solar_zenith_angle
    - IR_134
    masking_limit: null

And definition of a specific masking limit:

  nir_reflectance_no_masking:
    compositor: !!python/name:satpy.composites.NIRReflectance
    prerequisites:
      - name: IR_108
    optional_prerequisites:
    - solar_zenith_angle
    - IR_134
    masking_limit: 88.0

The default is to mask with the default limit defined in pyspectral.near_infrared_reflectance.TERMINATOR_LIMIT, which is also the current behaviour.

codecov · 2020-10-05T08:35:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@40fb438). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1390   +/-   ##
=========================================
  Coverage          ?   90.57%           
=========================================
  Files             ?      228           
  Lines             ?    33430           
  Branches          ?        0           
=========================================
  Hits              ?    30278           
  Misses            ?     3152           
  Partials          ?        0

Impacted Files	Coverage Δ
satpy/composites/__init__.py	`82.80% <100.00%> (ø)`
satpy/tests/test_composites.py	`99.89% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40fb438...2e20102. Read the comment docs.

coveralls · 2020-10-05T08:44:27Z

Coverage increased (+0.001%) to 90.566% when pulling 3230f85 on pnuu:feature-pyspectral-masking-limit into 40fb438 on pytroll:master.

coveralls · 2020-10-05T08:44:27Z

Coverage increased (+0.007%) to 90.571% when pulling 2e20102 on pnuu:feature-pyspectral-masking-limit into 40fb438 on pytroll:master.

pnuu · 2020-10-05T10:01:42Z

Before I forget. This test tested that the default keyword argument should not be None, but at the same time explicitly passed None as the value. I changed this to do what the method name said.

mraspaud · 2020-10-05T11:35:42Z

@pnuu this is the reason for that test: 53620fd#diff-c3f713f166b7940f26ca41873bcd1890R650-R655
So the objective was to check that the default value from the calculator was use if None was passed.

mraspaud · 2020-10-05T11:40:47Z

satpy/composites/__init__.py

-        if self.sun_zenith_threshold is not None:
-            reflectance_3x_calculator = Calculator(metadata['platform_name'], metadata['sensor'], metadata['name'],
-                                                   sunz_threshold=self.sun_zenith_threshold)
-        else:
-            reflectance_3x_calculator = Calculator(metadata['platform_name'], metadata['sensor'], metadata['name'])
-            self.sun_zenith_threshold = reflectance_3x_calculator.sunz_threshold
+        reflectance_3x_calculator = Calculator(metadata['platform_name'], metadata['sensor'], metadata['name'],
+                                               sunz_threshold=self.sun_zenith_threshold,
+                                               masking_limit=self.masking_limit)


I'm not sure this is right. When self.sun_zenith_threshold is None, we want to replace it with the default value from the calculator.
This was what #1319 was about.

Oh. That sounds wrong. I'd expect None to mean "do not apply SZA correction in the Calculator". I'd just fix the documentation to say If not explicitly specified, the default value defined in Pyspectral will be used.

But anyway, the behaviour on the Satpy side doesn't change: the default value is used if the user doesn't override it. If the user decides to override the default with None, they'll probably know what they are doing (which is, crash the code 😁).

ok, just change the __init__ docstring then :)

Docstring updated. Good thing, as I didn't remember to add the new kwarg there before.

pnuu · 2020-10-05T12:52:03Z

@pnuu this is the reason for that test: 53620fd#diff-c3f713f166b7940f26ca41873bcd1890R650-R655
So the objective was to check that the default value from the calculator was use if None was passed.

... which was because Satpy didn't respect the default and overrode it with None. This PR also fixes that by importing the defaults from Pyspectral.

mraspaud · 2020-10-05T13:00:13Z

Last question (maybe :) ): Should we use the pyspectral default or prevent potential problems if pyspectral is modified and hardcode a satpy-specific set of limits?

pnuu · 2020-10-05T13:04:32Z

I thought about that. Now that you raise the point again, I think that would make more sense than importing from Pyspectral. It would also make testing more explicit. I'll add Satpy-internal thresholds for these two arguments. Any suggestions what the default for masking_limit should be? Based on my experience with Day/Night composites I'd say masking_limit=None (no masking) is the best choice, but for "standalone" composites the "strict" limit might be better 🤔

mraspaud · 2020-10-05T13:15:47Z

ok, what if the masking limit is beyond the sunz_threshold, would that have the same effect as masking_limit set to None for Day/Night composites?

pnuu · 2020-10-05T13:29:10Z

It behaves the same as long as there's overlap between the two composites. It seems the default blending in DayNightCompositor is from 85.0 to 88.0 degrees SZA. So beyond 88.0 the NIRReflectance shouldn't matter at all. I'll try that limit and use it if seems to work as expected.

pnuu · 2020-10-05T13:42:35Z

Yep, The results are identical. Below are images with the 88.0 degree limit, first the single composite and then with night_microphysical on the night side. I'll update the tests and push the updates in a bit.

mraspaud

LGTM, just one stylistic question

mraspaud · 2020-10-05T14:15:28Z

satpy/composites/__init__.py

+NIR_REFLECTANCE_TERMINATOR_LIMIT = 85.0
+NIR_REFLECTANCE_MASKING_LIMIT = 88.0


Just to keep things gathered in the same place, would it be worth have these at class attributes to NIRReflectance?

Will do. Update follows in a minute or three.

Code and tests updated, tested with real data.

pnuu · 2020-10-05T14:35:31Z

Just as a reminder: the Pyspectral PR pytroll/pyspectral#113 needs to be merged (and released) first for this to be usable.

mraspaud · 2020-10-06T07:45:30Z

Probably one more thing to do here is to set the minimal version of pyspectral in setup.py

pnuu · 2020-10-06T07:46:57Z

Agreed. That'll need the new Pyspectral release to see the actual version number.

pnuu · 2020-10-07T04:43:57Z

Pyspectral version requirement updated to the release made yesterday.

pnuu · 2020-10-19T13:06:40Z

This PR isn't blocked anymore by the Pyspectral PR which has been merged and released.

Add support to Pyspectral NIRReflectance masking limit

3230f85

pnuu added enhancement code enhancements, features, improvements component:compositors labels Oct 5, 2020

pnuu requested review from mraspaud and djhoese October 5, 2020 08:28

pnuu self-assigned this Oct 5, 2020

mraspaud reviewed Oct 5, 2020

View reviewed changes

Update NIRReflectance docstring

bb1aa60

Use Satpys own defaults to NIRReflectance Calculator calls

b12d86d

mraspaud approved these changes Oct 5, 2020

View reviewed changes

Move NIRReflectance limits to class attributes, update tests

a3078cb

mraspaud added the blocked Waiting on an external action (eg release of other package) label Oct 5, 2020

Update pyspectral minimum version requirement

2e20102

pnuu removed the blocked Waiting on an external action (eg release of other package) label Oct 19, 2020

mraspaud merged commit 2948936 into pytroll:master Oct 19, 2020

pnuu deleted the feature-pyspectral-masking-limit branch October 21, 2020 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to Pyspectral NIRReflectance masking limit #1390

Add support to Pyspectral NIRReflectance masking limit #1390

pnuu commented Oct 5, 2020 •

edited by mraspaud

codecov bot commented Oct 5, 2020 •

edited

coveralls commented Oct 5, 2020

coveralls commented Oct 5, 2020 •

edited

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

mraspaud Oct 5, 2020

pnuu Oct 5, 2020

mraspaud Oct 5, 2020

pnuu Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

pnuu commented Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud left a comment

mraspaud Oct 5, 2020

pnuu Oct 5, 2020

pnuu Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud commented Oct 6, 2020

pnuu commented Oct 6, 2020

pnuu commented Oct 7, 2020

pnuu commented Oct 19, 2020

		NIR_REFLECTANCE_TERMINATOR_LIMIT = 85.0
		NIR_REFLECTANCE_MASKING_LIMIT = 88.0

Add support to Pyspectral NIRReflectance masking limit #1390

Add support to Pyspectral NIRReflectance masking limit #1390

Conversation

pnuu commented Oct 5, 2020 • edited by mraspaud

codecov bot commented Oct 5, 2020 • edited

Codecov Report

coveralls commented Oct 5, 2020

coveralls commented Oct 5, 2020 • edited

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

mraspaud Oct 5, 2020

Choose a reason for hiding this comment

pnuu Oct 5, 2020

Choose a reason for hiding this comment

mraspaud Oct 5, 2020

Choose a reason for hiding this comment

pnuu Oct 5, 2020

Choose a reason for hiding this comment

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud commented Oct 5, 2020

pnuu commented Oct 5, 2020

pnuu commented Oct 5, 2020

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud Oct 5, 2020

Choose a reason for hiding this comment

pnuu Oct 5, 2020

Choose a reason for hiding this comment

pnuu Oct 5, 2020

Choose a reason for hiding this comment

pnuu commented Oct 5, 2020

mraspaud commented Oct 6, 2020

pnuu commented Oct 6, 2020

pnuu commented Oct 7, 2020

pnuu commented Oct 19, 2020

pnuu commented Oct 5, 2020 •

edited by mraspaud

codecov bot commented Oct 5, 2020 •

edited

coveralls commented Oct 5, 2020 •

edited