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

Downscaling #173

Merged
merged 21 commits into from
Oct 13, 2020
Merged

Downscaling #173

merged 21 commits into from
Oct 13, 2020

Conversation

dnerini
Copy link
Member

@dnerini dnerini commented Sep 14, 2020

@dnerini dnerini marked this pull request as draft September 14, 2020 19:39
@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #173 into master will increase coverage by 0.22%.
The diff coverage is 86.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
+ Coverage   74.15%   74.38%   +0.22%     
==========================================
  Files         106      110       +4     
  Lines        8686     8814     +128     
==========================================
+ Hits         6441     6556     +115     
- Misses       2245     2258      +13     
Impacted Files Coverage Δ
pysteps/io/interface.py 94.73% <ø> (ø)
pysteps/tests/test_io_bom_rf3.py 100.00% <ø> (ø)
pysteps/tests/test_io_fmi_pgm.py 100.00% <ø> (ø)
pysteps/tests/test_io_knmi_hdf5.py 100.00% <ø> (ø)
pysteps/tests/test_io_mch_gif.py 100.00% <ø> (ø)
pysteps/tests/test_io_opera_hdf5.py 100.00% <ø> (ø)
pysteps/tests/test_io_saf_crri.py 86.04% <ø> (ø)
pysteps/visualization/precipfields.py 69.31% <ø> (ø)
pysteps/utils/images.py 67.12% <20.00%> (-1.90%) ⬇️
pysteps/io/exporters.py 43.86% <40.00%> (-0.37%) ⬇️
... and 12 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 a70c4ef...bc99172. Read the comment docs.

@dnerini
Copy link
Member Author

dnerini commented Sep 15, 2020

@jleinonen , I realize now that your implementation of rainfarm includes spatial downscaling only, as opposed to the Rebora et al 2006 paper, which also includes temporal downscaling.

This is not a problem, I'm just wondering what's the best way to proceed.

We could try to complete the code with the temporal part, but this risks to take some time which unfortunately I don't have at the moment.

We could simply specify in the docstring that this implementation is a simplified version which deals with spatial downscaling only. Hopefully the code will be completed and extended at some point.

What's your take on this?

@dnerini
Copy link
Member Author

dnerini commented Oct 3, 2020

This still misses some testing of the new module, but I'd say that we are anyway ready for a first round of review. Any feedback and suggestion for improvement is welcomed!

@jleinonen would also be great if you could have a quick look, thanks!

@dnerini dnerini marked this pull request as ready for review October 3, 2020 10:12
Nice work with the gallery! I added two comments, which may (hopefully) help the reader. Other than that, I've nothing to add.
Copy link
Contributor

@RubenImhoff RubenImhoff left a comment

Choose a reason for hiding this comment

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

Hi @dnerini,

This is a great contribution! I also like the gallery, a very clear example of the method.

I added two comment strings in the script for the gallery and I had two questions about the variables in rainfarm.py. So nothing major at all. :)
Regarding the rainfarm.py script, is it an idea to add some comment lines and perhaps refer to the equation numbers in the paper of Rebora et al? That may increase the readability of the script a bit.

Other than that, good to go!

Thanks!

@dnerini dnerini requested a review from jleinonen October 4, 2020 08:49
dnerini and others added 7 commits October 4, 2020 10:50
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
Co-authored-by: Andres Perez Hortal <andresperezcba@gmail.com>
@dnerini dnerini merged commit 8f0f07c into master Oct 13, 2020
@dnerini dnerini added this to the release v1.4 milestone Oct 22, 2020
@dnerini dnerini deleted the downscaling branch November 23, 2020 12:05
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.

4 participants