vtsuperdarn / davitpy

DEPRECATED The DaViT Python project
http://vtsuperdarn.github.com/davitpy/
GNU General Public License v3.0
37 stars 59 forks source link

SuperDARN Data Plotting Notebook and Underlying Functions #233

Closed w2naf closed 8 years ago

w2naf commented 8 years ago

Hi, guys.

Nice job on the new release! I'm just starting to use the new code now.

I found that some of the changes break things in the SuperDARN Data Plotting Notebook (https://github.com/vtsuperdarn/davitpy/blob/35c3de481ff4a12699401b390d7d4e387592ead9/docs/notebook/SuperDARN%20Data%20Plotting.ipynb). I tried fixing some of these problems myself last night, but ran into some issues I want to get feedback on before changing anything.

The main problem is that the figure keyword was removed from plotting.rti.plot_rti() (https://github.com/vtsuperdarn/davitpy/blob/35c3de481ff4a12699401b390d7d4e387592ead9/davitpy/pydarn/plotting/rti.py#L87). It looks like this was done because the new code loops through a frequency band filter and is then supposed to return a list of figures.

Next, the overlay of potential contours is now broken. I think this is because of bug in matplotlib.ticker.LinearLocator(), and filed and issue regarding that here: https://github.com/matplotlib/matplotlib/issues/6270

Thanks, Nathaniel

Shirling-VT commented 8 years ago

Hi Nathaniel,

I agree with you about the rti plot issue. I also find it inconvenient without the figure keyword when I tried to write a script to get more than one rti plot and save the figures for later usage. The lack of figure keyword prevents me from using of savefig function.

Best wishes, Xueling

On Wed, Apr 6, 2016 at 10:41 AM, Nathaniel Frissell < notifications@github.com> wrote:

Hi, guys.

Nice job on the new release! I'm just starting to use the new code now.

I found that some of the changes break things in the SuperDARN Data Plotting Notebook ( https://github.com/vtsuperdarn/davitpy/blob/35c3de481ff4a12699401b390d7d4e387592ead9/docs/notebook/SuperDARN%20Data%20Plotting.ipynb). I tried fixing some of these problems myself last night, but ran into some issues I want to get feedback on before changing anything.

The main problem is that the figure keyword was removed from plotting.rti.plot_rti() ( https://github.com/vtsuperdarn/davitpy/blob/35c3de481ff4a12699401b390d7d4e387592ead9/davitpy/pydarn/plotting/rti.py#L87). It looks like this was done because the new code loops through a frequency band filter and is then supposed to return a list of figures.

  • I personally advocate against this, as I think these plotting routines should just return a single object. That sort of looping should be done in a user-level script or a wrapper.
  • If we do want to keep this functionality, then there are some issues in plot_rti() we need to fix.

Next, the overlay of potential contours is now broken. I think this is because of bug in matplotlib.ticker.LinearLocator(), and filed and issue regarding that here: matplotlib/matplotlib#6270 https://github.com/matplotlib/matplotlib/issues/6270

Thanks, Nathaniel

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/vtsuperdarn/davitpy/issues/233

Best wishes, Xueling

asreimer commented 8 years ago

So what I'm getting from this is that the plot_rti function should only ever return one figure? I'm ok with that.

Anyone else care weigh in here?

MuhammadVT commented 8 years ago

Hey @asreimer

Would you mind modifying plot_rti as @w2naf and @Shirling-VT have requested? I also agree with them on this regard.

Thanks

asreimer commented 8 years ago

Hi @MuhammadVT,

I can do this. It should be a simple fix (famous last words). Give me a day or two.

MuhammadVT commented 8 years ago

Hey @asreimer

How are you counting your "a day or two"?

asreimer commented 8 years ago

After more than a few days, I have made a pull request (https://github.com/vtsuperdarn/davitpy/pull/266) that should address @w2naf and @Shirling-VT's concerns. I've also made the code a bit easier to read (and use I hope).

MuhammadVT commented 8 years ago

Addressed by #266.