gdsbook/book

fix: arg `geog` not used in function `g_map`, notebook 7

pareyesv opened this issue · 8 comments

In notebook 7, cell 24

The function:

def g_map(g, geog, ax)

does not use the arg geog. It only works with the geodataframe db instead.

Cell 25 works because geog = db (but actually db is hard-coded):

    ax = g_map(g, db, ax)

Let me know how to proceed. I can work on the PR (I just have to change the function g_map), however I don't know how to proceed (I couldn't find info for contributors, sorry). Should I change the ipynb? or the md instead?

Hello @pareyesv, thank you very much for contributing an issue to this book project!

On your question, I'm not sure I understand what you mean by "it does not use the arg geog"? The function g_map is defined on the Getis and Ord section and it includes a docstring with documentation to use it. I'll reproduce here for simplicity:

def g_map(g, geog, ax):
    '''
    Create a cluster map
    ...
    
    Arguments
    ---------
    g      : G_Local
             Object from the computation of the G statistic
    geog   : GeoDataFrame
             Table aligned with values in `g` and containing 
             the geometries to plot
    ax     : AxesSubplot
             `matplotlib` axis to draw the map on

    Returns
    -------
    ax     : AxesSubplot
             Axis with the map drawn
    '''
    ...

According to the documentation, geog represents the geo-table aligned with the Getis-Ord statistic object you pass in the first position and that we call here g. The documentation also states that geog is a GeoDataFrame object.

The function is later on in the chapter used to render a cluster map. In that case, we our GeoDataFrame object is called db and takes the order that, in the function, is assigned for geog.

I hope this helps but feel free to follow up if you're still running into issues. I'm closing this for now but please do reopen if needed.

Hi @darribas

Thanks for the reply.
Sorry if I didn't explain it properly...

In cell 24, the argument/parameter geog is never called/invoked inside the function g_map. In the screenshot below I highlighted all the appearances of geog.

image

It seems the signature should be:

def g_map(g, db, ax):

Notice that cell 25 works by chance (see the highlighted blue line) because db is hardcoded.

@darribas
Unfortunately, I cannot re-open my own issue if a repo collaborator closed it.
So I opened #253

Hi @pareyesv, thanks for following up on this. I think I see what you mean, but I don't think that is a bug in the book's code. The naming of geog is used in the context of the function definition and, since it is an object, it can be named in any other way when the function is executed, as long as it meets the requirements specified in the documentation. As an example, you could rename db as geog:

geog = db

And the code would work. But you don't have to. The point I'm trying to make is that geog is the name used in the definition of the function and, whatever object is passed later on for the argument, does not need to be named geog. Hope this makes sense.

Hi @pareyesv my apologies, I was too quick and did not read in detail, I'm sorry. I see what you mean now and yes it is an issue. PR incoming.

And over at #256, thanks for the suggestion and sorry again for the misunderstanding on my end!

No worries @darribas It's fine! Happy to collaborate!
Great book! One of the PhD students I'm working on is following the book and we noticed the issue.

By the way, I'm a researcher at Barcelona Supercomputing Center. We are working on urban planning, spatial data analysis, digital twins for cities.
In case you are interested, we will be more than happy to discuss possibilities of collaboration in the near future.

Saludos!

Fantastic! If you run into more bugs, please keep them coming!

On the suggestion to collaborate, could you please follow up offline (perhaps by email?) to keep things separated? Thanks!