vtsuperdarn / davitpy

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

Removed unnecessary plotCoords variable that caused bug. #296

Closed asreimer closed 7 years ago

asreimer commented 7 years ago

As discussed to in #286, the plotCoords in plotMapGrd.py is actually unnecessary. Not only that but it actually causes a big problem in the convection map plotting when setting plotCoords=mlt. For example, running this code in develop:

import datetime
import matplotlib.pyplot as plt
import davitpy.pydarn.plotting.plotMapGrd
from davitpy.utils import *
fig = plt.figure()
ax = fig.add_subplot(111)
sdate = datetime.datetime(2016, 3, 1, 9, 0)
mObj = plotUtils.mapObj(boundinglat=50., datetime=sdate,
                        gridLabels=True, coords='mlt')
mapDatObj = davitpy.pydarn.plotting.plotMapGrd.MapConv(sdate, mObj, ax,plotCoords='mlt')
mapDatObj.overlayMapFitVel()
mapDatObj.overlayCnvCntrs()
mapDatObj.overlayHMB()
plt.show()

will produce a groce (with a c) convection map plot: figure_1

After squashing this bug with this pull request, you should be able to make beautiful plots that look like this:

figure_2

The plotCoords keyword has been removed since it doesn't actually do anything anymore, so when you run your code on this pull request make sure you don't use it.

Shirling-VT commented 7 years ago

Confirmed the bug in the develop branch and if run the following codes in the bugfix_plotCoords_variable branch:

import datetime import matplotlib.pyplot as plt import davitpy.pydarn.plotting.plotMapGrd from davitpy.utils import * fig = plt.figure() ax = fig.add_subplot(111) sdate = datetime.datetime(2016, 3, 1, 9, 0) mObj = plotUtils.mapObj(boundinglat=50., datetime=sdate, gridLabels=True, coords='mag') mapDatObj = davitpy.pydarn.plotting.plotMapGrd.MapConv(sdate, mObj, ax) mapDatObj.overlayMapFitVel() mapDatObj.overlayCnvCntrs() mapDatObj.overlayHMB() plt.show()

image

import datetime import matplotlib.pyplot as plt import davitpy.pydarn.plotting.plotMapGrd from davitpy.utils import * fig = plt.figure() ax = fig.add_subplot(111) sdate = datetime.datetime(2016, 3, 1, 9, 0) mObj = plotUtils.mapObj(boundinglat=50., datetime=sdate, gridLabels=True, coords='mlt') mapDatObj = davitpy.pydarn.plotting.plotMapGrd.MapConv(sdate, mObj, ax) mapDatObj.overlayMapFitVel() mapDatObj.overlayCnvCntrs() mapDatObj.overlayHMB() plt.show()

image

import datetime import matplotlib.pyplot as plt import davitpy.pydarn.plotting.plotMapGrd from davitpy.utils import * fig = plt.figure() ax = fig.add_subplot(111) sdate = datetime.datetime(2016, 3, 1, 9, 0) mObj = plotUtils.mapObj(boundinglat=50., datetime=sdate, gridLabels=True, coords='mlt') mapDatObj = davitpy.pydarn.plotting.plotMapGrd.MapConv(sdate, mObj, ax) mapDatObj.overlayGridVel() mapDatObj.overlayCnvCntrs() mapDatObj.overlayHMB() plt.show()

image

asreimer commented 7 years ago

Excellent.

Since @Shirling-VT and I worked on this together, it's probably a good idea for someone else to test this code and merge it.

aburrell commented 7 years ago

Looking through the plotMapGrd.py file there are some changes that need to be made before merging:

1) MapConv description no longer match input (reference to plotCoords variable still present) 2) Error statement, "Map object is using one hemisphere and data the other" could be changed to be more clear, perhaps: "Map and data objects must be from the same hemisphere" 3) overlayGridVel, calcFitCnvVel descriptions are incomplete 4) nitpick: are we really going to default to jet? https://jakevdp.github.io/blog/2014/10/16/how-bad-is-your-colormap/ 5) Parenthesis should be added on 199 so my brain doesn't hurt when I look at it: vecLen = velsPlot[nn] * self.lenFactor / self.radEarth / 1000.

Ran the tests and was able to reproduce all three plots. Don't know why the text is so small, it didn't look that way on the display.

dtest1 dtest2 dtest3

ksterne commented 7 years ago

Wow, it's been awhile! Sorry my code development time has been taken up by the demands happening in RST world. I've added two commits here to address @aburrell's 1 and 2.

For 3, I see this is outside of the scope of this bugfix. Yes it should be corrected, but doesn't relate here.

For 4, I think this would take some kind of central SuperDARN authority on what default color schemes would be....good luck on finding or being able to organize that. I think the idea has been that slow motions are cool colors while hot colors represent fast motion.

For 5, @aburrell, agreed, even though this is outside of the scope, it may be worth it. Do you feel comfortable placing the parentheses in the correct spots?

Otherwise, I've tested this and see that it's working. So aside from the last comment here, this is ready to merge.

ksterne commented 7 years ago

I should also note that the latest dependencies update throws a lot of depreciation warnings from matplotlib. So, we'll be looking at a good overhaul of code to clear those out.

aburrell commented 7 years ago

I went to make the changes, but I can't push commits to this pull request. So, whilst I made the changes, no I can't add them here.

ksterne commented 7 years ago

Post it here and I can push it up?

aburrell commented 7 years ago

Not practical. Better to just leave it for a different pull request, since parens won’t affect the code functionality.

On 19 Apr 2017, at 13:24, Kevin Sterne notifications@github.com wrote:

Post it here and I can push it up?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vtsuperdarn/davitpy/pull/296#issuecomment-295377972, or mute the thread https://github.com/notifications/unsubscribe-auth/AGuC_g05C3TTcVj2qllkFCNo1Y020Uroks5rxlFBgaJpZM4LlKdW.

ksterne commented 7 years ago

Sounds good. Merging it now. I'll try to get to the aacgm pull request before next Thursday's release branch creation.