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

Paper: PyDDA: A New Pythonic Multiple Doppler Retrieval Package #474

Open
wants to merge 19 commits into
base: 2019
from

Conversation

Projects
None yet
5 participants
@rcjackson
Copy link

commented May 22, 2019

This paper shows a new Python package for retrieving winds from radar networks called PyDDA.

@rcjackson

This comment has been minimized.

Copy link
Author

commented May 22, 2019

Not sure why it doesn't build. The build completes successfully on my machine.

@deniederhut

This comment has been minimized.

Copy link
Member

commented May 22, 2019

GitHub has a backend outage earlier which may have been the issue. Looks fine now!

@ocefpaf

ocefpaf approved these changes Jun 7, 2019

Copy link

left a comment

My final review is to approved the paper. All my comments are minor and/or clarification questions/suggestions.

The paper describes the development a python package for solving a problem in a domain within the scope of the SciPy conference. All the code present could be easily verified (I did not run all the examples though).

I did not verified the final PDF for length due to laptop limitations at the moment (travelling with a Windows machine). My review was based only on the rst file here.


.. class:: abstract

PyDDA is a new community framework aimed at wind retrievals that depends

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

More of a question than a review comment but it is unclear to me why a specialized package instead of pushing for metadata standard and using a "generic" package to fetch the data. I'm Thinking about OPeNDAP/ERDDAP and CF-1.7. (Bare in mind I'm not a meteorologist nor I deal with wind data on a day-to-day basis.)

Answering my own question as I read more below. The word "retrievals" threw me off. This is calculating winds from data. (Quite interesting BTW!)

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

In the meteorology community, a “retrieval” is defined as calculating a quantity from a set of other quantities that are recorded by the instrument. For example, if you want to retrieve sea surface temperature from satellite, this is not detected directly, but rather derived from irradiances from various channels scanned from the satellite. In this case, we are deriving three dimensional winds from the radial velocities detected by the radar. This therefore explains our choice of wording here.

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 24, 2019

I guess that as an oceanographer I prefer calculate rather than retrieve 😄

is not scalable. In addition, the addition of constraints from other 3D
fields such as models is not supported. Finally, Multidop is a wrapper
around a program written in C which introduces issues related to packaging
in frameworks such as anaconda and scalability due to the non-threadsafe

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Should it be non-thread-safe? Ugly but I don't think the word threadsafe exists.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have changed this to say “non-thread-safe” as that is the actual spelling of the word.

Introduction
------------

Three dimensional wind retrievals are important for examining the dynamics

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

I have to say that, after reading just the abstract of the paper mentioned here, the authors did an amazing job on avoiding/explain jargon!

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

Our goal is to make this work accessible to the general scientific community, so we avoided the use of or explained jargon as it was needed.

is not scalable. In addition, the addition of constraints from other 3D
fields such as models is not supported. Finally, Multidop is a wrapper
around a program written in C which introduces issues related to packaging
in frameworks such as anaconda and scalability due to the non-threadsafe

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

A minor nitpick but anaconda has compilers and makes packaging C code less painful. I would just say "written in C which introduces issues related to packaging."

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have changed the phrasing of this sentence to the reviewer’s suggestion.

combined with data from model reanalyses with just a few lines of code. In addition,
PyDDA is built upon the Python ARM Radar Toolkit (Py-ART) :cite:`HelmusandCollis2016`.
Since Py-ART is already used by hundreds of users in the radar meteorology community, these
users would be able to learn how to use PyDDA easily. In addition, the open source nature

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Suggestion: two "In addition" in a row. Change one to something like "moreover."

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have changed the phrasing of this sentence to the reviewer’s suggestion.


In addition, PyDDA includes 4 different initialization routines that will
create this field for you from various data sources. In particular,
PyDDA even supports the ECMWF web API for the automatic retrieval

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

The word "retrieval" here is used in the sense of downloading, right?

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have retaiored this whole section to focus more on the steps used to create the initializations and constraints in PyDDA at the suggestion of @nicholasmalaya. In doing this, this sentence was eliminated.

import numpy as np
def calculate_mass_continuity(

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

I like the code example but I have a question: does this take a lot of space in the final PDF? If so, should it be in an appendix? We have the equations and the code example is only to show how PyDDA does it.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

This code takes almost half of a page of the PDF, so we decided to move this code to the appendix so for the interested reader.

out_grids = pydda.retrieval.get_dd_wind_field_nested(
mygs, ui, vi, wi, Co=1.0, Cm=100.0,
Cmod=1e-5, model_fields=["hrrr", "erainterim"],
client=client)

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Suggestion: I would just mention that you can pass a dask client instance to PyDDA instead of the code snippet. (Or maybe putting the code snippet in an Appendix or an external link to it.)

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have removed the code snippet in this section and simply mention that a dask Client can be passed into PyDDA for the nested retrieval.

Tornado in Sydney, Australia using 4 radars
-------------------------------------------

.. figure:: australian_radar_layout.png

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Suggestion: re-create the figure without cartopy's stock image. At this zoom level the pixels maybe be confused with relevant data, or use a higher resolution version of the image. (My first impression was: where is the colorbar, until I realized that was the stock-image.)

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have removed the cartopy stock figure from the image.

.. figure:: Sydney_tornado.png
:align: center

A quiver plot inside a supercell that spawned a tornado in the vicinity of

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Can you increase the scale of the quiver plot arrows? They are barely visible.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

The size of the quiver arrows has been increased.

and versions of the documentation in non-English languages.

All contributions to PyDDA will have to be submitted by a pull request to the master branch
on https://github.com/openradar/PyDDA. From there, the main developers will examine the pull

This comment has been minimized.

Copy link
@ocefpaf

ocefpaf Jun 7, 2019

Suggestion: I feel that the directions to the package should be in the first paragraph instead of buried in the "Contributor Information."

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have added a URL to the package to the abstract and the introduction.

@nicholasmalaya

This comment has been minimized.

Copy link

commented Jun 11, 2019

I agree with @ocefpaf that this is a good submission to SciPy, and should be strongly considered for acceptance after clarifications and minor revisions. Thank you @rcjackson and co-authors!

A few board, major comments:

  • Are the examples provided? I don't see a link to them.
  • Should this PDF work in black and white? I'm of two minds. The pictures are pretty and viewing them on a computer isn't too unusual. In general as a reviewer I recommend papers work in black and white, however.
  • Similar to above, the plots aren't particularly friendly to the color-blind. Perhaps changing the contours to dot-dashed would help.
  • Is the variational problem regularized? Is this an option one should choose?

I'll put more detailed comments in the .tex directly.


The evaluation of :math:`J(\textbf{V})` can be done entirely using calls
from NumPy and SciPy. For example, evaluating :math:`J_{c}(\vec{\textbf{V}}) =
\nabla\cdot\vec{\textbf{V}})` with an optional anelastic term be reduced to a few NumPy calls:

This comment has been minimized.

Copy link
@nicholasmalaya

nicholasmalaya Jun 11, 2019

Small typo here: missing left paranthesis. I recommend:

Suggested change
\nabla\cdot\vec{\textbf{V}})` with an optional anelastic term be reduced to a few NumPy calls:
(\nabla\cdot\vec{\textbf{V}})` with an optional anelastic term be reduced to a few NumPy calls:

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have fixed this parenthesis.

demonstrates what kinds of contributions to PyDDA would be useful. As of the writing
of this paper, the road map states that the current goals of PyDDA are to implement:

* Support for a greater number of high resolution (LES) models such as CM1

This comment has been minimized.

Copy link
@nicholasmalaya

nicholasmalaya Jun 11, 2019

Citation for CM1?

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We have added a citation to Bryan and Fritsch 2002 which is the original paper for CM1:

Bryan, G.H. and J.M. Fritsch, 2002: A Benchmark Simulation for Moist Nonhydrostatic Numerical Models. Mon. Wea. Rev., 130, 2917–2928, https://doi.org/10.1175/1520-0493(2002)130<2917:ABSFMN>2.0.CO;2

+------------------+----------------------------------------------+
| Data source | Routine in constraints module |
+==================+==============================================+
| Weather Research | :code:`make_constraint_from_wrf` |

This comment has been minimized.

Copy link
@nicholasmalaya

nicholasmalaya Jun 11, 2019

I'd prefer citations for these model constraints, or defining what they are in the text. This is important for reproducability, e.g. stating a model or constraint is seldom sufficient to define it for a user. I'd also recommend a single sentence for each be added to the document (or, better, the table label) that notes roughly what these model constraints are.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

Inserting models as constraints into variational wind retrievals is a new feature of PyDDA and is something that has not been commonly done in multiple-Doppler retrievals. Therefore, we have decided to provide further details about how both the initializations and constraints are constructed. The details provided include how the data are interpolated to the analysis grid and then how the extra term related to the model constraint in the cost function is constructed when performing the optimization.

+-----------------+----------------------------------------------+
| Data source | Routine in initialization module |
+=================+==============================================+
| Weather | |

This comment has been minimized.

Copy link
@nicholasmalaya

nicholasmalaya Jun 11, 2019

I'd prefer citations for these initializations. I'd also recommend a single sentence for each be added to the document (or, better, the table label) that notes roughly what these initializations are.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

Similar to the previous comment we have added more details about how the initializations from model data are constructed. In the table caption we have added:

“These constraints are constructed by interpolating the model :math:J(\vec{\textbf{V}}) to the analysis grid coordinates.”

In addition, in the following paragraph we have stated how the model initialization is constructed by interpolating the three components of the wind field from the model onto the analysis grid using nearest-neighbor interpolation and then using that as the initial guess for the optimization loop.

However, this also demonstrates the success in integrating radar data
from 3 radars and a high resolution
reanalysis to provide the most complete wind retrieval possible.

This comment has been minimized.

Copy link
@nicholasmalaya

nicholasmalaya Jun 11, 2019

I recommend you add a new section here. It would be 'verification' and discuss what regression testing, correctness checks, etc. exist in PyDDA. After all, how can users know if this library is functioning correctly? This is important for reproducibility of the work.

This comment has been minimized.

Copy link
@rcjackson

rcjackson Jun 24, 2019

Author

We run a suite of 27 tests on PyDDA using pytest. Every time a pull request to the master branch of PyDDA is requested, Travis CI will run this suite of tests to ensure that PyDDA is functioning correctly. We have added a section on verification that both states that we run this suite of tests and gives a few examples of the unit tests that are performed on PyDD. Examples of such unit tests check to see if the mass continuity cost function is nonzero in a converging wind field, the smoothness cost function is nonzero in a discontinuous wind field, and if the variational technique converges to the model input if only the model field and constraint are used. This helps to ensure that the variational technique is working as intended.

@cchoirat

This comment has been minimized.

Copy link

commented Jun 11, 2019

I mostly have minor comments about the paper (from the PDF version I retrieved initially), which would be a good submission for SciPy.

  • There are repetitions, on p.1., for example, "to use as it uses" or "In addition, the addition"

  • Sentences such as "This therefore permits the easy installation of PyDDA using pip of anaconda. Given that alone is a major hurdle to [...]", I think, read the opposite of of they actually mean.

  • p. 4, "Figure using a similar" : Figure number is missing.

@rcjackson

This comment has been minimized.

Copy link
Author

commented Jun 24, 2019

I am going to place the general response to the 3 reviewers here, with responses to their more specific comments in the relevant comment sections in the thread above.

@ocepaf:
We thank the reviewer for taking the time to provide his valuable feedback. We have responded to the individual comments using GitHub’s commenting system.

@nicholasmalaya:
We thank @nicholasmalaya for his valuable comments. They have considerably improved the manuscript.
We have included the source code and the links to the data in all of the examples in this document here.
https://openradarscience.org/PyDDA/source/auto_examples/index.html.

The figures have been regenerated to be more readable in response to all of the reviewers’ comments. In particular, the contours in the plots have been switched to hatches. In addition, we now use @CameronHomeyer’s colormap that visualizes radar reflectivity fields using a color-blind friendly scheme which will make the figures more accessible. This colormap is now used in Figures 2, 3, 4, 6, and 8. In addition, the sizes of the quivers and streamline arrows have been enhanced in the new versions of Figure 1 and 6, making these figures more readable. Finally, Figure 6 now uses a hatched contour instead of a transparent contour to denote the regions where updrafts are present.

In terms of trying to make this paper readable in black and white, it is no longer usual in the meteorology community to attempt to make radar images work in black and white as they are very difficult to visualize effectively without color. In essence, any colormap we use has to highlight regions of light rain, moderate rain, heavy rain, and hail all in ways that are easily distinguishable. This is very difficult to do effectively in black and white. Therefore, in order to better highlight significant features of the storm, we have decided that color figures are the best way to visualize the data. The use of color-blind friendly colormaps does help the figures appear better in black and white.

The variational problem is regularized in two different ways in order to ensure a physically realistic solution that is free of noise. One is through the use of a hard box constraint on the solution when executing the L-BFGS-B minimization technique. Throughout the domain, the wind velocities are constrained to be within -100 to 100 m/s which is a physically realistic scenario for virtually all weather that occurs on Earth. In addition, in order to reduce noise in the final solution, there are options to have a low pass filter as well as adjusting the smoothness constraint that is directly proportional to the Laplacian of the wind field. We have added a few sentences in the paper that mention these techniques.

@cchoirat:

We thank the reviewer for taking the time to review the paper and providing her valuable comments. We have embedded our responses below.

There are repetitions, on p.1., for example, "to use as it uses" or "In addition, the addition"
The repetitiveness of the introduction was duly noted by all reviewers so some changes here were made to improve the readability of this section For example, we have changed the reviewer’s first example of repetition to state “While CEDRIC was revolutionary for its time, it is difficult to use as a separate scripting language is the input for the retrieval.” In addition, the first “in addition” was replaced with “moreover” following the suggestion of @ocepaf.

Sentences such as "This therefore permits the easy installation of PyDDA using pip of anaconda. Given that alone is a major hurdle to [...]", I think, read the opposite of of they actually mean.

To make this clearer we have replaced “this” with “installation” in this sentence.

p. 4, "Figure using a similar" : Figure number is missing.

We have fixed the figure number here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.