1. Marc R.
  2. mss
  3. Pull requests

GSOC2019 funnelferry

#643 Merged at e67b94a
Repository
funnelferry
Branch
GSOC2019-funnelferry
Repository
wxmetvis
Branch
GSOC2019-funnelferry
Author
  1. Anveshan Lal
Reviewers
Description
  • converted function requiring basemap to cartopy in utils.py

Comments (10)

  1. Joern Ungermann

    This is a good start. I am not sure that we also need to get rid of pyproj, which is also available outside matplotlib, but the less dependencies, the better.

    Do you know if the npts_cartopy routine is called in a time-critical setting? The python code is potentially much slower than a more optimized implementation. Please try if the moving of waypoints by mouse in a stereographic projection still draws the flightpath sufficiently fast after applying the changes requested below.

    1. Anveshan Lal author

      I was confused about pyproj and thinking it is just a part of basemap (which is also seperately available) I thought to replace its use,

      I removed the for loop and computed the points with help of direct function alone but I made a big mistake of just testing the function standalone with values rather than opening the application and it turns out there is a bug

      File "lib/cartopy/geodesic/_geodesic.pyx", line 188, in cartopy.geodesic._geodesic.Geodesic.inverse
TypeError: float() argument must be a string or a number, not 'datetime.datetime

      and there is not a chance for it to be faster than code involving pyproj, so would you suggest reverting back to it(pyproj) at the cost of one extra dependency or make it work with Cartopy itself?

      1. Joern Ungermann

        No, cartopy should be able to replace pyproj. I was a bit surprised to see that cartopy does not make use of pyproj, but it makes sense to not only remove basemap, but also pyproj.

        As such it does not need to be faster, but should not be much slower. Without the for loop, it looks better.

        Opening and using the program is kind of the minimum you need to do for testing.

        Better is adding a unit test for the function you are modifying.

        And you should execute the full testbench regularily and make sure that it passes both without and with your changes.

        ‌

        The error message you pasted does not contain enough information to identify the problem. The full log is required.

          1. Joern Ungermann

            Sorry, but I do not understand you.

            The log shows clearly that cartopy is given a datetime instead of a coordinate. It is understandable that it fails.

            The trace back shows that this is triggered by the SideView. Having a look at the code along the trace shows that a coordinate with three entries is prepared, lon/lat/datetime

            As the error does not occur without your changes (I checked), your changes are responsible. There are not that many, so identifying the culprit is easy.

            Having a look at your changes shows that you added a np.flip where before explicitely coordinates 0 and 1 were adressed.

            In this way, you feed the third entry of the lon/lat/datetime coordinate as lat to cartopy, which fails.

            ‌

            This fixes it:

            diff --git a/mslib/utils.py b/mslib/utils.py
index 674bbda..7fd0800 100644
--- a/mslib/utils.py
+++ b/mslib/utils.py
@@ -114,10 +114,8 @@ def get_distance(coord0, coord1):
     Returns:
         length of distance in km
     """
-    coord0 = np.flip(coord0) # function is provided coordinates in lat/lon
-    coord1 = np.flip(coord1) # but cartopy takes input in lon/lat format
     pr = gd.Geodesic()
-    return (pr.inverse(coord0, coord1).base[0,0] / 1000.)
+    return (pr.inverse([coord0[1], coord0[0]], [coord1[1], coord1[0]]).base[0,0] / 1000.)


 def find_location(lat, lon, tolerance=5):
@@ -604,7 +602,7 @@ def writexml(self, writer, indent=u"", addindent=u"", newl=u""):
     else:
         writer.write(u"/>%s" % (newl))

-def ntps_cartopy1(coord1, coord2, numpoints):
+def npts_cartopy(coord1, coord2, numpoints):
     distance = get_distance(coord1, coord2)/(numpoints + 1)
     new_geo = gd.Geodesic()
     all_distance = [distance*i*1000 for i in range(1, numpoints + 1)]

            ‌

            I assume that you worked on other places on the replacement of basemap while being blocked here. Do you have someting to show there that I can comment on?

            1. Anveshan Lal author

              Thank you, I am sorry, should’ve spent more time observing the arguement of function through the log. Currently I am changing the code of plot_hsection of mpl_hsec.py, and would have a pr up by tomorrow as soon as possible along with tests for new function added to utils.py

              1. Joern Ungermann

                Good.

                As a side note, you should also start looking out for following PEP8 formatting.

                By executing

                python setup.py test --addopts "--flake8 -m flake8"

                on the commandline you get a list of errors. You should try to fix the given warnings and errors.

                Most editors can be configured to either highlight PEP8 violations or automatically reformat the code.

                1. Anveshan Lal author

                  @Joern Ungermann I know I am behind schedule but I may need more time for next PR and would work on weekends to make-up for lost progress.

                  I was a little stuck to emulate the “area_thresh” property of a basemap instance onto cartopy but couldn’t figure out if we can or not? By my understanding area_thresh defines the smallest area below which a coastline or closed area won’t be plotted.

                  1. Joern Ungermann

                    I would not necessarily say your are already behind, but keep an eye on not getting blocked by a single issue for too long. You need to build up problem-solving skills such that you do not rely solely on more experienced people’s expertise, but also not waste too much time on something that can be trivially solved.

                    area_thresh seems like an non-essential feature. If you cannot replicate it easily, skip it for the time being. We can re-evaluate it later, but from a user-perspective it is probably negligible and from a server-operator perspective, we need to measure the performance later on. But cartopy is supposed to be faster than basemap, generally, so I am confident that this will be unproblematic as well.

                    I prefer a rapid-prototyping approach. That is, first get it to run somehow, and then sort out all the little short-cuts you took one by one according to priority. So anything that generates a reasonable plot in the backend would be sufficient for the first step.

                  2. Joern Ungermann

                    Also, can you finish this PR by applying the changes I proposed, such that we can advance to the next one? I’d like to see your progress or non-progress on the server side cartopy migration in a new Merge request as soon as possible so I can support you.