Skip to content

Conversation

@sjsrey
Copy link
Member

@sjsrey sjsrey commented May 19, 2025

No description provided.

sjsrey added 2 commits May 19, 2025 09:07
- Add rng machinery to distance tests
- Add rng seed inside _ripley_test
- Docstrings for seeds
- Pass rng around to downstream calls to poisson in strauss
- Tests for strauss
@codecov
Copy link

codecov bot commented May 19, 2025

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.5%. Comparing base (c4203b6) to head (93afa24).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
pointpats/random.py 95.7% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #160     +/-   ##
=======================================
+ Coverage   74.5%   80.5%   +6.0%     
=======================================
  Files         12      12             
  Lines       1904    1999     +95     
=======================================
+ Hits        1419    1609    +190     
+ Misses       485     390     -95     
Files with missing lines Coverage Δ
pointpats/distance_statistics.py 83.2% <100.0%> (+0.2%) ⬆️
pointpats/random.py 88.4% <95.7%> (+61.7%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say much about the Strauss process but we should revisit the seed/rng logic to follow SPEC7.

metric="euclidean",
hull=None,
edge_correction=None,
seed=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use seed anymore but rng see SPEC7 - https://scientific-python.org/specs/spec-0007/

Comment on lines 152 to 157
seed : int or None, default=None
Seed for initializing the random number generator if `rng` is not provided.
Has no effect if `rng` is explicitly passed.
rng : numpy.random.Generator or None, default=None
A NumPy random number generator. If None, a new generator is created
using `numpy.random.default_rng(seed)`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this does not follow SPEC7, which is unfortunate for a new code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I will refactor to respect SPEC7.

@jGaboardi
Copy link
Member

Can't say much about the Strauss process but we should revisit the seed/rng logic to follow SPEC7.

@sjsrey – regarding Martin's comment here, from what I can tell all the seed keywords are new. So if that the case, shall simply remove them from this PR?

(this will also close #114)

@sjsrey
Copy link
Member Author

sjsrey commented Jul 22, 2025

Can't say much about the Strauss process but we should revisit the seed/rng logic to follow SPEC7.

@sjsrey – regarding Martin's comment here, from what I can tell all the seed keywords are new. So if that the case, shall simply remove them from this PR?

(this will also close #114)

Simply removing seed won't work here. Instead we need to refactor this a bit to get the correct behavior.

I can do this for pointpats, but I'm wondering if we want to revisit moving random tooling up into libpysal to ensure consistency throughout the federation?

@martinfleis
Copy link
Member

I can do this for pointpats, but I'm wondering if we want to revisit moving random tooling up into libpysal to ensure consistency throughout the federation?

where would that be consumed and what would it contain?

@sjsrey
Copy link
Member Author

sjsrey commented Jul 22, 2025

I can do this for pointpats, but I'm wondering if we want to revisit moving random tooling up into libpysal to ensure consistency throughout the federation?

where would that be consumed and what would it contain?

where:

  • esda
  • inequality
  • giddy
  • segregation
  • pointpats
  • spopt
    (maybe others)

in terms of what it would contain in libpysal, something like:

import numpy as np

def get_rng(random_state=None):
    """
    Return a NumPy Generator instance using Scientific Python SPEC 7 conventions.

    Parameters
    ----------
    random_state : int, numpy.random.Generator, or None
        - If int: used as seed to create a Generator
        - If Generator: returned as-is
        - If None: a new Generator is created

    Returns
    -------
    numpy.random.Generator
    """
    return np.random.default_rng(random_state)

Then to use it in the downstream packages:

from libpysal import get_rng

rng = get_rng(random_state)
rng.permutation(...)

Might be overkill to centralize this, but the key thing is to adopt a consistent pattern across the ecosystem for spec7.

@sjsrey sjsrey closed this Jul 22, 2025
@sjsrey sjsrey reopened this Jul 22, 2025
@jGaboardi
Copy link
Member

Might be overkill to centralize this, but the key thing is to adopt a consistent pattern across the ecosystem for spec7.

Overkill? Maybe.

Ideal for centralization & consistency? Absolutely yes IMHO

Getting everyone on board to use the centralized functionality? Good luck LOL

@martinfleis
Copy link
Member

It is a one-liner wrapper in another one-liner. I'd rather vote for transparency of using directly np.random.default_rng than hiding it behind get_rng which does not do anything else than passing the arg and returning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants