search

ceholden / yatsm

Yet Another Time Series Model
https://yatsm.readthedocs.org/en/latest/
MIT License
63 stars 30 forks source link

Fixes to issue #88 #89

Closed parevalo closed 8 years ago

ceholden commented 8 years ago

The test failure is a testament to how unstable CCDC results can be relative to its input parameters.

The test failed because of the location in which you put the self._update_model call. In your PR it's before the self.here += 1 happens so there is a difference of 1 observation in the model fit compared to the master branch version. This 1 extra observation in the model fits resulted in a change that was originally not detected, presumably because the estimated model parameters (coefficients or RMSE) were altered just enough.

The version of CCDC I've targeted (~ v10) fits all models right away when it enters the monitoring period -- meaning that it does move self.here += 1 before estimating the models. The change you made results in a different behavior that can be fixed by moving the self._update_model call as indicated in the comment on the code I added. The pixel used for the test does have a massive land cover change in ~1999, but the reference result also finds a change in ~2003. The change in 2003 shows up in the NIR very well, but not very well in the red/SWIR. Finding only 1 change is arguably a preferred behavior, but who knows what other changes it outside of this single test time series!

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.01%) to 78.888% when pulling 2db111f51649a2176cf335cdf2cb489f43fe7aa5 on parevalo:issue88 into 195fe4256d06d6d2b824fcd8ce1ace09072be83d on ceholden:master.

ceholden commented 8 years ago

Great -- I'll write up the test case for the bail behavior and we'll be good to go

Tests added in 97ffe1650de945c6aa73b68e57c5cae007afb0fc