search

pytroll / pygac

A python package to read and calibrate NOAA and Metop AVHRR GAC and LAC data
https://pygac.readthedocs.org/
GNU General Public License v3.0
20 stars 27 forks source link

Replace cython filter #83

Closed carloshorn closed 4 years ago

carloshorn commented 4 years ago

This PR closes #74.

carloshorn commented 4 years ago

As side note on performance, this implementation is more efficient than the previous one, because it only requires to run once through the rolling windows.

mraspaud commented 4 years ago

Nice work! I'm wondering why you replaced mean with std? And there seems to be handling for fill values in the previous implementation, is it supported here too?

mraspaud commented 4 years ago

Also should we wait for @sfinkens to run his tests before merging this?

carloshorn commented 4 years ago

I'm wondering why you replaced mean with std?

The heart of the correct_tsm_issue module is the get_tsm_idx method: https://github.com/pytroll/pygac/blob/5c59f2be9437ad7bfc0005038f45f8416d10828d/pygac/correct_tsm_issue.py#L436-L455

which requires the std_filter. The mean_filter was just an implementation detail as explained in the doc-string of the std_filter, https://github.com/pytroll/pygac/blob/5c59f2be9437ad7bfc0005038f45f8416d10828d/pygac/correct_tsm_issue.py#L417 Which shows how to build a std_filter by calling twice a mean_filter.

And there seems to be handling for fill values in the previous implementation, is it supported here too?

The fill value was again an implementation detail to handle NaN values, see https://github.com/pytroll/pygac/blob/5c59f2be9437ad7bfc0005038f45f8416d10828d/pygac/correct_tsm_issue.py#L399 Since bottleneck.nanstd takes care of the NaN values in this PR, there is no need for it anymore.

Also should we wait for @sfinkens to run his tests before merging this?

Yes please, because I have seen differences at machine precision comparing the std_filter implementations, so he could probable comment on the impact. I do not expect any concerns, but in addition it would be nice to include @sfinkens in changing a module that he worked on.

mraspaud commented 4 years ago

@sfinkens you need to run your tests on this.

sfinkens commented 4 years ago

Will run tests ASAP

sfinkens commented 4 years ago

Regression tests pass!

mraspaud commented 4 years ago

All good then, merging