functional reached calculations #575

u3ks · 2024-04-29T14:03:28Z

a new functional version of the mm.Reached function. It calculates several statistics at once similar to mm.describe(

u3ks · 2024-04-29T14:18:04Z

Timings:

count without spatial weights

old: 24.5 ms ± 216 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
new: 15.4 ms ± 368 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

count with spatial weights

old: 2.59 s ± 33.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
new: 322 ms ± 6.73 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

statistics with spatial weights:
-old: 1min ± 601 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
-new: 322 ms ± 6.73 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

martinfleis

First round

martinfleis · 2024-04-29T14:06:18Z

momepy/functional/_diversity.py

+try:
+    from numba import njit
+except (ModuleNotFoundError, ImportError):
+    warnings.warn(
+        "The numba package is used extensively in this function to accelerate the"
+        " computation of statistics but it is not installed or  cannot be imported."
+        " Without numba, these computations may become slow on large data.",
+        UserWarning,
+        stacklevel=2,
+    )
+    from libpysal.common import jit as njit


The warning should be printed only if a user calls a function that actually uses numba. That is why it was originally inside describe. I would still keep the warning inside if possible.

martinfleis · 2024-04-29T14:10:21Z

momepy/functional/_diversity.py

+
+
+def describe_reached(
+    y, y_group, result_index=None, graph=None, q=None, include_mode=False


I would probably call y_group graph_index following what we did in street_alignment

momepy/momepy/functional/_distribution.py

Lines 318 to 322 in 36b13f8

def street_alignment(

building_orientation: Series,

street_orientation: Series,

street_index: Series,

) -> Series:

it should be clear from the name that this contains ID's aligned with y that link to the index in graph.

martinfleis · 2024-04-29T14:11:46Z

momepy/functional/_diversity.py

+    y, y_group, result_index=None, graph=None, q=None, include_mode=False
+) -> DataFrame:
+    """
+    Calculates statistics of ``y`` objects reached on a street network.


Not only street network. Graph or IDs can come from anywhere. You can easily use enclosure ID here instead. The function is pretty generic.

martinfleis · 2024-04-29T14:12:34Z

momepy/functional/_diversity.py

+    """
+    Calculates statistics of ``y`` objects reached on a street network.
+    Requires a ``y_group`` that links the ``y`` objects to streets (or ``graph``)
+    assigned beforehand (e.g. using :py:func:`momepy.get_network_id`).


Suggested change

assigned beforehand (e.g. using :py:func:`momepy.get_network_id`).

assigned beforehand (e.g. using :py:func:`momepy.get_nearest_street`).

that is the new one

martinfleis · 2024-04-29T14:14:36Z

momepy/functional/_diversity.py

-    return stat_
+    if isinstance(y, np.ndarray):
+        y = pd.Series(y, name="obs_index")
+    elif isinstance(y, Series):


Suggested change

elif isinstance(y, Series):

if isinstance(y, Series):

to treat the series created above, no?

martinfleis · 2024-04-29T14:16:10Z

momepy/functional/_diversity.py

+    )
+    result.loc[stats.index.values] = stats.values
+    result.columns = stats.columns
+    result = result.fillna(0)


No need for this if you are filling the results full of zeros, no?

martinfleis · 2024-04-29T14:17:09Z

momepy/functional/tests/test_diversity.py

@@ -11,7 +12,7 @@
 GPD_013 = Version(gpd.__version__) >= Version("0.13")


-class TestDistribution:
+class TestDiscribe:


Suggested change

class TestDiscribe:

class TestDescribe:

typo

martinfleis · 2024-04-29T14:18:31Z

momepy/functional/tests/test_diversity.py

+        # not using assert_result since the method
+        # is returning an aggregation, indexed based on nID


Why not? We use it in describe without issues. I'd prefer to use it as it checks the index.

codecov · 2024-04-29T15:22:00Z

Codecov Report

Attention: Patch coverage is 98.88889% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 97.8%. Comparing base (4037c70) to head (e21ca5e).
Report is 20 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #575     +/-   ##
=======================================
+ Coverage   97.4%   97.8%   +0.4%     
=======================================
  Files         26      37     +11     
  Lines       4328    5299    +971     
=======================================
+ Hits        4214    5183    +969     
- Misses       114     116      +2

Files	Coverage Δ
momepy/functional/tests/conftest.py	`100.0% <ø> (ø)`
momepy/functional/tests/test_diversity.py	`100.0% <100.0%> (ø)`
momepy/functional/_diversity.py	`97.8% <97.6%> (ø)`

... and 13 files with indirect coverage changes

martinfleis · 2024-04-30T10:24:43Z

ci/envs/310-oldest.yaml

@@ -10,7 +10,7 @@ dependencies:
  - networkx=2.7
  - numpy=1.22
  - packaging
-  - pandas>=1.4.0,!=1.5.0,<2
+  - pandas=2.0


Can you revert this since we're checking for 2.1.0 anyway?

martinfleis · 2024-04-30T10:26:19Z

momepy/functional/_diversity.py

+throw_numba_warning = False
+try:
+    from numba import njit
+except (ModuleNotFoundError, ImportError):
+    throw_numba_warning = True
+    from libpysal.common import jit as njit


Suggested change

throw_numba_warning = False

try:

from numba import njit

except (ModuleNotFoundError, ImportError):

throw_numba_warning = True

from libpysal.common import jit as njit

try:

from numba import njit

HAS_NUMBA = True

except (ModuleNotFoundError, ImportError):

HAS_NUMBA = False

from libpysal.common import jit as njit

Just a minor nit to keep this consistent across PySAL.

martinfleis · 2024-04-30T11:20:52Z

momepy/functional/_diversity.py

+    results = [
+        values.shape[0],
+        np.mean(values),
+        np.median(values),
+        np.std(values),
+        np.min(values),
+        np.max(values),
+        np.sum(values),


Needs to work properly with missing values.

I updated the code, I think we can just get rid of the nans beforehand and compute everything on the non_na values, what do you think?

jGaboardi · 2024-05-02T16:18:18Z

momepy/functional/_diversity.py

+    Parameters
+    ----------
+    grouper : pandas.GroupBy
+        Groupby Object which specifies the aggregations to be performed


Suggested change

Groupby Object which specifies the aggregations to be performed

Groupby Object which specifies the aggregations to be performed.

jGaboardi · 2024-05-02T16:23:19Z

momepy/functional/_diversity.py

+    """
+
+    if Version(pd.__version__) <= Version("2.1.0"):
+        raise NotImplementedError("Please update to a newer version of pandas.")


Is this a proper usage of a NotImplementedError?

should be ImportError I think

jGaboardi · 2024-05-02T16:25:11Z

momepy/functional/_diversity.py

+
+    if (result_index is None) and (graph is None):
+        raise ValueError(
+            "One of result_index or graph has to be specified, but not both."


Let's declare this ValueError so it can be used here and below. Or at least declare the message to re-use.

martinfleis

Few notes, mostly on the documentation side.

momepy/functional/_diversity.py

martinfleis · 2024-05-13T08:53:56Z

momepy/functional/_diversity.py

+    """
+    Calculates statistics of ``y`` objects reached on a neighbourhood graph.
+    Requires a ``graph_index`` that links the ``y`` objects to ``graph`` or streets


It do not have to be streets, the graph can be done based on anything. The key point is that it is a Graph based on another object than the one on which we have y. We can include link to streets as an example.

martinfleis · 2024-05-13T08:55:29Z

momepy/functional/_diversity.py

+    The statistics calculated are count, sum, mean, median, std.
+    Optionally, mode can be calculated, or the statistics can be calculated in
+    quantiles ``q``.


Can you move this to the top and have the implementation details come after this? You first want to know what it does and then how to use it.

momepy/functional/_diversity.py

Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>

functional reached calculations

0d79d29

martinfleis reviewed Apr 29, 2024

View reviewed changes

jGaboardi assigned u3ks Apr 29, 2024

jGaboardi added the refactor label Apr 29, 2024

u3ks added 2 commits April 29, 2024 16:27

update pandas version in latest ubuntu image

6cd4767

added test versioning

181c278

changes based on PR

84ab18a

martinfleis reviewed Apr 30, 2024

View reviewed changes

u3ks added 2 commits May 2, 2024 13:08

changes based on PR

92d2dfd

env rollback

ffad7a4

martinfleis requested a review from jGaboardi May 2, 2024 16:08

jGaboardi reviewed May 2, 2024

View reviewed changes

u3ks added 2 commits May 6, 2024 13:37

pr changes and nan stat test

a94d178

typing and test versioning check

d33ef23

martinfleis reviewed May 13, 2024

View reviewed changes

u3ks and others added 2 commits May 13, 2024 13:45

Apply suggestions from code review

f32747f

Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net>

documentation changes

e21ca5e

martinfleis approved these changes May 13, 2024

View reviewed changes

martinfleis merged commit 889fb44 into pysal:main May 13, 2024
14 checks passed

martinfleis added enhancement New feature or request and removed refactor labels Jun 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

functional reached calculations #575

functional reached calculations #575

u3ks commented Apr 29, 2024 •

edited

Loading

u3ks commented Apr 29, 2024

martinfleis left a comment

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

martinfleis Apr 29, 2024

codecov bot commented Apr 29, 2024 •

edited

Loading

martinfleis Apr 30, 2024

martinfleis Apr 30, 2024

martinfleis Apr 30, 2024

u3ks May 2, 2024

jGaboardi May 2, 2024

jGaboardi May 2, 2024

martinfleis May 2, 2024

jGaboardi May 2, 2024

martinfleis left a comment

martinfleis May 13, 2024

martinfleis May 13, 2024



		def describe_reached(
		y, y_group, result_index=None, graph=None, q=None, include_mode=False

	def street_alignment(
	building_orientation: Series,
	street_orientation: Series,
	street_index: Series,
	) -> Series:

	assigned beforehand (e.g. using :py:func:`momepy.get_network_id`).
	assigned beforehand (e.g. using :py:func:`momepy.get_nearest_street`).

		# not using assert_result since the method
		# is returning an aggregation, indexed based on nID

	Groupby Object which specifies the aggregations to be performed
	Groupby Object which specifies the aggregations to be performed.

functional reached calculations #575

functional reached calculations #575

Conversation

u3ks commented Apr 29, 2024 • edited Loading

u3ks commented Apr 29, 2024

martinfleis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinfleis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

u3ks commented Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 29, 2024 •

edited

Loading