search

CSU-Radarmet / CSU_RadarTools

A module of independent functions to do precipitation retrievals from polarimetric radar data
GNU General Public License v2.0
88 stars 43 forks source link

ENH: Add fundamental attenuation calculations. #35

Closed nguy closed 8 years ago

nguy commented 8 years ago

This is an experimental addition of some fundamental calculations. This first file indicates the rough structure of the files to included (6 or 7 total at the end). Let me know if this looks okay in terms of structure, etc. for CSU toolkit.

It would also take modifying the setup.py to initialize them. One possibility is to create a directory called fundamentals or such and putting these all under that heading. I have no strong feelings on either direction.

nguy commented 8 years ago

I should also explicitly note to not accept until the rest is added if you decide to move forward.

tjlang commented 8 years ago

Thanks, I'll take a look at this soon.

tjlang commented 8 years ago

Looks like a good start. I made on comment on the code itself. I think the init.py file in this directory should not need to be changed. However, you will need to update the README and the documentation in the setup.py file so that users know this sub-module exists. I might recommend that this all be kept in a single sub-module even if the content expands beyond attenuation. That may require a name change and/or a separate sub-directory (with associated init.py) to house everything.

nguy commented 8 years ago

How about one large file that is called fundamentals.py?

In addition, I have a conversions that I'd like to rename common.py and add the _check_array function into. Are you okay with that?

tjlang commented 8 years ago

Yeah, that works. I can make use of _check_array elsewhere too.

nguy commented 8 years ago

@tjlang lot's of changes, they were already staged. See what you think.

Also, should I roll the rev number?

nguy commented 8 years ago

Oh and two more things:

  1. I'm open to changing names of any functions etc.
  2. I didn't connect the linearize or _check_for_array functions in existing code yet
tjlang commented 8 years ago

Looking good. This is almost there. We need to decide what to do with the common module. I feel it should not be externally advertised, only used internally. Or perhaps the whole thing should be merged with fundamentals. Another thought is we only put functions in it that are used directly in other modules, but this may require some reshuffling between fundamentals and common. I'm OK with merging both now, as long as we don't advertise common in the docs for now. We can figure out its ultimate role later. Also, I can work in the internal imports (e.g., linearize, zdp, etc.) later.

Other than that, I'd recommend some pep8 cleanup. Also, please consider adding a section on fundamentals to the demo notebook. There's too much in there to do a thorough review, but maybe you could do a brief demo of 2-3 functions in it. Just put it after the csu_misc section.

Note - I've updated the notebook recently so you'll need to merge the latest pull before you modify.

Sound OK?

Thanks a bunch, this looks to be a great addition!

nguy commented 8 years ago

Oops, sorry about the pep8. I meant to run that, but forgot. It passes pep8 on my system now. Hopefully this works better.

I agree with common being removed from the docs and outside. I originally had this as a conversion.py, which ultimately could be put in place just like fundamentals I think. But that can be decided later as you noted.

Examples added to notebook.

tjlang commented 8 years ago

Looks good. Merging, and then we can fine tune from there.