Moar RTI fixes

[asreimer]

This pull request further refactor the RTI plotting code and removes the multiple figure plotting associated with the tfreqbands keyword. The tfreqbands keyword is changed to txfreq_lims.

plot_rti now always returns a figure handle if the plotting was successful.

This code still needs testing. I recommend at least 2 other people also test this code to find any remaining bugs. There is also some PEP8 compliance stuff needed to be fixed.

This was made to address concerns raised in https://github.com/vtsuperdarn/davitpy/issues/233.

[MuhammadVT]

Hey @asreimer ,

Thank you for this pull request.

I have tested it with almost all possible argument values and It worked fine. Just a small issue here, retfiq argument is mentioned in the doctrings but does not seem to be used anywhere.

BTW, the issue in #186 is still there.

Here is the test code I have used:

import datetime as dt from davitpy.pydarn.plotting import plot_rti from davitpy.pydarn.sdio.radDataRead import radDataOpen import matplotlib.pyplot as plt

stm = dt.datetime(2014,2,20) etm = dt.datetime(2014,2,21) rad = "bks"

ftype = "fitacf"

ftype = "fitacf" bmnum = 13

scales = [[-150, 150]]

params = ["velocity"]

params = ["power", "velocity", "width"] coords='mag'

gsct=True

gsct=False filtered=False channel=None

channel="a"

plot_terminator=True

fig = plot_rti(stm, rad, eTime=etm, bmnum=bmnum, fileType=ftype, params=params, coords=coords, gsct=gsct, filtered=filtered, channel=channel, show=False, low_gray=True, plot_terminator=plot_terminator)

plot the data from a file

ffname = "/tmp/sd/20140220.000000.20140221.000000.bks.fitacf" myPtr = radDataOpen(stm, rad, eTime=etm, bmnum=bmnum, fileName=ffname, fileType=ftype) fig = plot_rti(stm, rad, eTime=etm, bmnum=bmnum, fileType=ftype, myFile=myPtr, fileName=ffname, params=paramimport datetime as dt from davitpy.pydarn.plotting import plot_rti from davitpy.pydarn.sdio.radDataRead import radDataOpen import matplotlib.pyplot as plt

stm = dt.datetime(2014,2,20) etm = dt.datetime(2014,2,21) rad = "bks"

ftype = "fitacf"

ftype = "fitacf" bmnum = 13

scales = [[-150, 150]]

params = ["velocity"]

params = ["power", "velocity", "width"] coords='mag'

gsct=True

gsct=False filtered=False channel=None

channel="a"

plot_terminator=True

fig = plot_rti(stm, rad, eTime=etm, bmnum=bmnum, fileType=ftype, params=params, coords=coords, gsct=gsct, filtered=filtered, channel=channel, show=False, low_gray=True, plot_terminator=plot_terminator)

plot the data from a file

ffname = "/tmp/sd/20140220.000000.20140221.000000.bks.fitacf" myPtr = radDataOpen(stm, rad, eTime=etm, bmnum=bmnum, fileName=ffname, fileType=ftype) fig = plot_rti(stm, rad, eTime=etm, bmnum=bmnum, fileType=ftype, myFile=myPtr, fileName=ffname, params=params, coords=coords, gsct=gsct, plot_terminator=plot_terminator)

plt.show() s, coords=coords, gsct=gsct, plot_terminator=plot_terminator)

plt.show()

[MuhammadVT]

My copy pasted test code is duplicated for some reason. Sorry for that.

[Shirling-VT]

I test it as well, it seems to work well with plot_rti, but not plotRti sometimes. I would recommend either remove the plotRti function or keep it consistent with plot_rit in the keywords and default values in those keywords, or it will cause some problem. One of the example is that in the notebook when do Local File Rti plot, if we choose (which is the true in our present notebook) plotRti, it will give you an AssertionError. Also it's confusing with two different functions there with the same functionalities.

On Tue, Jul 26, 2016 at 11:46 AM, Muhammad Rafiq notifications@github.com wrote:

My copy pasted test code is duplicated for some reason. Sorry for that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vtsuperdarn/davitpy/pull/266#issuecomment-235310994, or mute the thread https://github.com/notifications/unsubscribe-auth/AIEZKybn47ZEDWndqBpgFGpMQ0UKwuS1ks5qZivogaJpZM4JUufN .

Best wishes, Xueling

[MuhammadVT]

Also, an import statement for num2date is missing.

[ksterne]

Hey @asreimer,

You'll see I got some PEP8 edits done here. Aside from getting away from camelCase, why was the tFreqBands parameter changed to txfreq_lims? Also, using this parameter breaks things here. I'm getting and error at line 307 noting the tbands is not defined using code similar to what @MuhammadVT posted.

[asreimer]

Alright, I've finally been able to make some time for this.

First, @MuhammadVT I'm not trying to fix #185 here. Someone should, but it should be done carefully as discussed in that issue's thread. I've also removed retFig from the docstring.

Next @Shirling-VT , I have removed plotRti. This was kept for backwards compatibility reasons, but we always intended to remove it. Since it's causing some confusion now and we were deprecating it anyway, I've now removed it. We changed to plot_rti since we don't want to use CamelCase for function names as part of the code style we've agreed to.

After that, I've added the missing num2date import.

@ksterne I've fixed the code that was causing you to have an error due to undefined tbands variable. That was a typo that should have just been tband. As for renaming tFreqBands, I changed it to move away from CamelCase but also to make the keyword more descriptive. Now it's explicitly clear that the keyword limits the tx frequencies that will be plotted.

I've also made changes to the docstring in a few places where it was inconsistent. Also, there was a type error in the logging statement that spits out how long it took to make the plot. I've fixed that too.

Finally, I've modified the RTI plotting such that one can specify a matplotlib colormap instead of only 'aj' and 'lasse'.

For example, give this a try:

fig = pydarn.plotting.plot_rti(datetime(2013,3,2),'rkn',eTime=datetime(2013,3,2,12),colors=['jet','jet_r','jet'])

And you should get this:

or on matplotlib 1.5.2:

fig = pydarn.plotting.plot_rti(datetime(2013,3,2),'rkn',eTime=datetime(2013,3,2,12),colors=['viridis','jet_r','viridis'])

[ksterne]

Sounds good on the small fixes here. Making the RTI plotter the best is awesome since this is our bread and butter. I've pushed the cpid limit section as well as did a few more PEP8 cleanups.

I guess that's ok to just remove the plotRti routine. Did we have a version we were going to remove that on? Just wondering if we made a promise to keep it around until x.x and now we're breaking that.

I see the txfreq_lims is now working and I can solve #186 using these, but this is because of better knowledge of things than the average user would have. I think I'm fairly good and approve this, even though we may want to better enhance the cpid panel limit.

[asreimer]

There was some comments in the plotRti code that said remove in 0.6, so I think we're ok here.

Also, I'm good with waiting for the CPID panel limit thing to be implemented in this pull request if we want to wait for that.

[ksterne]

Ok, by request I set the default at 20, but added a cpidchange_lims parameter so that someone can change it around if needed. Hows that look?

[asreimer]

Hey @ksterne I've tested the cpidchange_lims keyword and it works just fine. I've left a note in the code though (https://github.com/vtsuperdarn/davitpy/pull/266#discussion_r76894305) about the CPID change error text moving around depending on the value of cpidchange_lims.

Everyone else, is there any other problems with this pull request? If not we should get this merged.

[ksterne]

Hey @asreimer, good catch on the placement of the text there! I've made the change as you were thinking, I think that will decrease the likelihood that the text runs off the right side of the panel and makes it unreadable.

[asreimer]

Hey @ksterne, the code works after your changes. Perhaps we should center the text in the middle of the CPID window. I'm going to modify the code to do that, we can change it back if others disagree.

[aburrell]

Hi Ashton,

Why not save yourself some time and put up two plots for comparison? Then we can vote.

Cheers, Angeline

On 8 Sep 2016, at 19:20, Ashton Reimer notifications@github.com wrote:

Hey @ksterne https://github.com/ksterne, the code works after your changes. Perhaps we should center the text in the middle of the CPID window. I'm going to modify the code to do that, we can change it back if others disagree.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vtsuperdarn/davitpy/pull/266#issuecomment-245690669, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_sbPQVq55r5y_XAlOIjoR6cUPeqiks5qoFHkgaJpZM4JUufN.

[asreimer]

Too late.

[asreimer]

I will post a before and after figure though (that's a good idea) to make it easier for people to decide if the changes I made should be reverted or not.

Before my changes:

After my changes:

[aburrell]

Looks like the best way to handle the issue. I’d say we should do the same thing for frequency, but that’s a bit more complicated.

Formatting wise, could we remove the “/“ from the date? It’s not correct to use them when the months aren’t numerals. Spaces should do just fine.

On 8 Sep 2016, at 21:10, Ashton Reimer notifications@github.com wrote:

I will post a before and after figure though (that's a good idea) to make it easier for people to decide if the changes I made should be reverted or not.

Before my changes: https://cloud.githubusercontent.com/assets/4604819/18364955/c687f932-75cd-11e6-8597-bd9dad6e7614.png After my changes: https://cloud.githubusercontent.com/assets/4604819/18364954/c6841fc4-75cd-11e6-8465-1cd0e4bd7278.png — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/vtsuperdarn/davitpy/pull/266#issuecomment-245724374, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_lvl_5UEluNuKjFPnDSJXQllHOSPks5qoGu-gaJpZM4JUufN.

[MuhammadVT]

Hey @asreimer,

I have a question not very much related to this pull request. Why do we have the 4 (minutes) hardcoded here?

# Build a list of datetimes to plot each data point at.
dt_list = []
for i in range(len(data_dict['times'][fplot])):
    x[tcnt] = matplotlib.dates.date2num(data_dict['times'][fplot][i])
    dt_list.append(data_dict['times'][fplot][i])

    if(i < len(data_dict['times'][fplot]) - 1):
        if(matplotlib.dates.date2num(
                **data_dict['times'][fplot][i + 1]) - x[tcnt] > 4. / 1440.):**
            tcnt += 1
            # 1440 minutes in a day, hardcoded 1 minute step per data point
            # but only if time between data points is > 4 minutes
            x[tcnt] = x[tcnt - 1] + 1. / 1440.
            dt_list.append(matplotlib.dates.num2date(x[tcnt]))
    tcnt += 1

[asreimer]

Hey @aburrell, I agree, we could change that formatting easily. I'll do it eventually unless someone beats me to it.

@MuhammadVT sorry I didn't see your message until now. Been buried under getting ready for a new job and writing a paper and my PhD thesis haha.

Excuses aside, the reason 4 minutes is hardcoded in there seems to be related to the old 2 minute scan modes that SuperDARN used to run. The pcolormesh function needs a bounding box to plot the RTI data into. The code you posted is responsible for making "coordinates" for little rectangles for pcolormesh to use. Usually these coordinates can be determined from the scan to scan. Unfortunately, we don't include the start and end time for any data in our data files (we do include the integration time, but that's only 3 seconds typically and would make RTIs look VERY bad), so the code you posted looks at the start time of the next scan and uses it as the end time for the previous scan. Sometimes there are gaps in the data, so instead of plotting a huge smear of colours, the code has a hardcoded 4 minutes into it to make only a small smear. 4 minutes was chosen because it is twice as long as the longest scan that was typically run on SuperDARN in the past (they used to run 2 minute scans using a 7 pulse sequence instead of the current 1 minute scans with an 8 pulse sequence).

[ksterne]

Hey @aburrell, I've changed the slashes to be spaces here as you were recommending/stating before. Is there anything else that we're not liking here?

I think the hardcoded 4 minute issue is a little outside of the original goal of this pull request. Is there any other reason not to merge this pull request in?

[aburrell]

@ksterne that looks good. One more step towards producing plots I wouldn't send back to an author for being poor publication quality.

I agree that the 4 minute issue should be addressed, but not in this pull request. @MuhammadVT , would you like to do the honours of creating a new issue? That way we don't forget about this again. It should be a simple fix (adding a keyword argument that defaults to 4 minutes and a reasonable explanation in the routine description), but deserves it's own pull request.

[aburrell]

@ksterne , you're the person who's done the most on this that is not currently moving house. Are you happy to merge?

[ksterne]

Sounds good, I'm happy to move it along. I think we've had a lot of progress with this pull request even though as it's been noted that it started out as something small and almost turned into "let's fix everything with RTI". Merging now...

[asreimer]

I agree that we made a lot of progress with this pull request. It ended up being a bit of a monster, but overall the RTI plotting is much better now!