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

Flexible time steps #191

Merged
merged 43 commits into from
Dec 9, 2020
Merged

Flexible time steps #191

merged 43 commits into from
Dec 9, 2020

Conversation

pulkkins
Copy link
Member

@pulkkins pulkkins commented Dec 3, 2020

This pull request aims to address issue #176. The main changes are:

  • Previously the output of the methods in the nowcasts module were constrained to have the same time step with the inputs. Now the user can supply a list of arbitrary output time steps.
  • Allow using extrapolation.semilagrangian.extrapolate with precip=None to integrate along the advection field without interpolating the precipitation field.

@codecov
Copy link

codecov bot commented Dec 3, 2020

Codecov Report

Merging #191 (ac8148a) into master (cba5f08) will increase coverage by 0.19%.
The diff coverage is 88.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   75.67%   75.86%   +0.19%     
==========================================
  Files         123      123              
  Lines        9273     9450     +177     
==========================================
+ Hits         7017     7169     +152     
- Misses       2256     2281      +25     
Flag Coverage Δ
unit_tests 75.86% <88.08%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pysteps/extrapolation/interface.py 86.66% <ø> (ø)
pysteps/nowcasts/interface.py 94.44% <ø> (ø)
pysteps/nowcasts/extrapolation.py 14.28% <14.28%> (-1.10%) ⬇️
pysteps/extrapolation/semilagrangian.py 70.65% <72.72%> (+1.24%) ⬆️
pysteps/nowcasts/anvil.py 69.37% <83.87%> (-1.27%) ⬇️
pysteps/nowcasts/utils.py 97.22% <84.61%> (-2.78%) ⬇️
pysteps/nowcasts/sseps.py 87.79% <90.00%> (+0.26%) ⬆️
pysteps/nowcasts/steps.py 85.79% <94.28%> (+1.63%) ⬆️
pysteps/nowcasts/sprog.py 90.30% <94.73%> (+0.67%) ⬆️
pysteps/tests/test_nowcasts_anvil.py 87.50% <100.00%> (+0.54%) ⬆️
... and 7 more

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 cba5f08...f1772d3. Read the comment docs.

@dnerini dnerini added this to the release v1.4 milestone Dec 3, 2020
@dnerini
Copy link
Member

dnerini commented Dec 3, 2020

Great work, @pulkkins !

So, how did you implement the computation of the AR process with irregular timesteps? Did you use the time-weighted interpolation that you proposed in #176?

Did you face any particular challenge to implement this to all the pysteps nowcasting systems?

@pulkkins
Copy link
Member Author

pulkkins commented Dec 4, 2020

Thanks @dnerini!

Yes, I used linear interpolation between the AR time steps, including the perturbation fields in STEPS. Apart from some technical work, I didn't face any major difficulties.

Copy link
Member

@dnerini dnerini left a comment

Choose a reason for hiding this comment

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

@pulkkins I went through the code, and I only found very minor suggestions (many of them simply repeat across all nowcasting methods, which explains the number of comments somewhat large…).

I only have one major concern: I included for all nowcast methods (anvil, sseps, steps) an additional test for cases when a list of timesteps is passed (bbd8aeb). My expectation was that the result (in terms of skill) would be equivalent to passing the corresponding number of timesteps (i.e., timesteps=3 vs timesteps=[3]), but for sseps and steps, this doesn't seem to be the case (the skill is lower, see failed test results). Is this to be expected because of the stochastic nature of the two method?

pysteps/extrapolation/semilagrangian.py Outdated Show resolved Hide resolved
pysteps/extrapolation/semilagrangian.py Outdated Show resolved Hide resolved
pysteps/extrapolation/semilagrangian.py Outdated Show resolved Hide resolved
pysteps/nowcasts/anvil.py Outdated Show resolved Hide resolved
pysteps/nowcasts/anvil.py Outdated Show resolved Hide resolved
pysteps/nowcasts/steps.py Outdated Show resolved Hide resolved
pysteps/nowcasts/steps.py Outdated Show resolved Hide resolved
pysteps/nowcasts/steps.py Outdated Show resolved Hide resolved
pysteps/nowcasts/steps.py Outdated Show resolved Hide resolved
pysteps/nowcasts/steps.py Outdated Show resolved Hide resolved
@pulkkins
Copy link
Member Author

pulkkins commented Dec 5, 2020

I only have one major concern: I included for all nowcast methods (anvil, sseps, steps) an additional test for cases when a list of timesteps is passed (bbd8aeb). My expectation was that the result (in terms of skill) would be equivalent to passing the corresponding number of timesteps (i.e., timesteps=3 vs timesteps=[3]), but for sseps and steps, this doesn't seem to be the case (the skill is lower, see failed test results). Is this to be expected because of the stochastic nature of the two method?

Good that you raised that concern. I need to do more testing to make sure that the new time step scheme works as intended in all cases. By the way, are you sure that the tests you are referring to are correct?

Line 85 of test_nowcasts_steps.py:
crps = verification.probscores.CRPS(precip_forecast[-1], precip_obs[-1])

The first index of precip_forecast refers to the ensemble member, not time step. Should that be precip_forecast[:,-1]?

@dnerini
Copy link
Member

dnerini commented Dec 5, 2020

I only have one major concern: I included for all nowcast methods (anvil, sseps, steps) an additional test for cases when a list of timesteps is passed (bbd8aeb). My expectation was that the result (in terms of skill) would be equivalent to passing the corresponding number of timesteps (i.e., timesteps=3 vs timesteps=[3]), but for sseps and steps, this doesn't seem to be the case (the skill is lower, see failed test results). Is this to be expected because of the stochastic nature of the two method?

By the way, are you sure that the tests you are referring to are correct?

Line 85 of test_nowcasts_steps.py:
crps = verification.probscores.CRPS(precip_forecast[-1], precip_obs[-1])

The first index of precip_forecast refers to the ensemble member, not time step. Should that be precip_forecast[:,-1]?

You're absolutely right! This could actually explain the failing tests: with timesteps=[3]the "ensemble" is smaller (essentially one single field, instead of the three fields with timesteps=3) and so produce the higher CRPS!
I'll fix it right now and see if the results align this way...

edit: That worked (see 79ac28d)! Nowcasting using timesteps=3 or timesteps=[3] produces the same skill! Brilliant!

@dnerini
Copy link
Member

dnerini commented Dec 8, 2020

Thanks for the last changes, @pulkkins , this now looks ready to be merged, well done!

@pulkkins pulkkins merged commit 5bbf573 into master Dec 9, 2020
@pulkkins pulkkins deleted the flexible_time_steps branch December 9, 2020 15:12
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.

None yet

2 participants