Conversation
…ethod p-value combiner - ewstools/spatial.py: morans_i(), morans_i_permutation_test(), SpatialEWS class following the existing MultiTimeSeries conventions. - ewstools/pvalues.py: combine_pvalues_ebm(), a real implementation of Poole et al. (2016)'s Empirical Brown's Method. - 21 new tests, 0 regressions on the existing 32. - CONTRIBUTION_spatial_significance.md documents the identified gap. Local branch, not yet submitted as a pull request.
…liques - ewstools : pull request réellement soumise (ThomasMBury/ewstools#482), statut passé de "développé, pas encore soumis" à "soumis (pull request ouverte)". - hopfieldkit : dépôt GitHub public créé (https://github.com/C95234/hopfieldkit), lien ajouté à la page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hi @C95234 — thanks for this, and apologies that it sat quiet for a few days. That's on me. I've read it properly now. You did the things that make a contribution easy to take seriously: you checked whether the gap was real before writing anything, the tests are substantive rather than decorative, you matched the conventions already in the package, and the design follows Dakos et al. (2010) closely. Moran's coefficient at distance 1, plus Kendall tau against the control parameter, is exactly that protocol. I had a look at Hélios as well. Pushing early-warning methods out into domains they weren't developed on is a particular interest of mine, too, so the cross-domain framing is one I actively care about. There are a few things I'd want to work through with you — a couple of them scientific rather than cosmetic — but none of them change my interest in the direction you're taking. I'm going through it carefully and talking it over with Tom, and I'll come back early next week with something specific. Thanks for taking the trouble to contribute this. — Bruce |
|
Hi @energyscholar — happy to help however's useful here. |
|
Sorry this is later than "early next week" — I wanted to check the implementation properly rather What holds upI went through this carefully, and the core of it is right.
I also ran a mutation check on The one thing I'd want addressed before merge: there is no trend controlThis is a library gap rather than a mistake in your code, and it is worth stating carefully because Moran's I is computed on the raw field at each time point. Moran's I is spatially mean-centred, so a
Dakos et al. control this by construction — they know what their model is doing. A library The consequence for the PR is that The good news is that the package already has the operation.
I'll add that as a documented recipe beside your PR, together with a shipped null and positive Two honest qualifications, both of which I'll put in the docstring rather than leave implied:
And one interpretation point, which is why the docstring I'm proposing reads conditionally rather A p-value that can be exactly zeroIn One line fixes it: "p_value": float((1 + np.sum(null_values >= observed)) / (1 + n_permutations)),That floors it at One note, not a request
|
…ues.py Applying Bruce Stephenson's (energyscholar) review of this PR, in full: - Permutation p-value floored at 1/(n_permutations + 1) (Davison & Hinkley 1997; North, Curtis & Sham 2002) so it can never land on exactly 0. - Renamed test_spatialews_compute_ktau_detects_increasing_trend -> test_spatialews_ktau_rises_under_strengthening_gradient_without_trend_control and marked it explicitly as a NEGATIVE CONTROL (it demonstrates a confound, not a real detection) -- while at it, also moved its fixture from 3x3/T=30 (passes only 526/1000 seeds -- essentially a coin flip) to the 6x6/T=60/>0.30 configuration Bruce validated at 1000/1000 seeds, verified independently here on another 500 seeds before committing. - Added a documented trend-control recipe and a weights-parameter caveat to the SpatialEWS docstring. - ewstools/spatial.py now exported from ewstools/__init__.py, and added to docs/source/ewstools.rst so it appears in the built docs. - Added the NaN-warning behaviour (and its test) to both compute_moran and compute_moran_significance. - Dropped ewstools/pvalues.py, tests/test_pvalues.py and CONTRIBUTION_spatial_significance.md per Bruce's scope call: combining significance across indicators belongs outside this package (CONTRIBUTING draws the line at indicators). It also had a real bug (anti-conservative under negative correlation) and a naming error (it's Kost & McDermott's method, not Brown's) -- both fixed in a corrected copy kept outside this repository. Full suite: 46 passed, 1 skipped (0 regressions on the 32 that existed before this PR). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for this — genuinely one of the most careful reviews I've had on anything, and the mutation testing especially is not something I'd have thought to do myself. Applied the checklist in the latest commit:
Full suite: 46 passed, 1 skipped, 0 regressions on the 32 that existed before this PR. Everything above I did myself rather than leaving it to you — happy to take a pass at any of the "follow-ups, not merge conditions" list too if useful, just say which. 🤖 Generated with Claude Code |
…eference-period note Not merge conditions per Bruce's review, but small enough to include now rather than leave for a separate pass: - lattice_weights(n_rows, n_cols=None, connectivity="rook"|"queen"): a convenience for the common regular-grid case, with a pointer to libpysal.weights for irregular real-world networks -- the split Bruce suggested (an arbitrary weights matrix IS the irregular-network generalisation; what was missing was just a grid builder). - esda.moran.Moran interop pointer in the SpatialEWS docstring, for users who want analytic single-snapshot inference rather than a trend across time. - Reference-period note: Moran's I depends on the unit set and weights as much as on the field, so a single value needs a reference period to be meaningful. 4 new tests for lattice_weights. Full suite: 50 passed, 1 skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Also went ahead and added the three follow-ups from your "not merge conditions" list, since they were small enough to fold in now rather than leave for later:
Full suite: 50 passed, 1 skipped. 🤖 Generated with Claude Code |
energyscholar
left a comment
There was a problem hiding this comment.
Checked the new head properly rather than on trust: 50 passed / 1 skipped locally, the
trend-control recipe runs verbatim (tau +0.68 -> -0.07 on the strengthening gradient), and the
new fixture holds on 1000/1000 seeds. Thank you for all of it, the fixture change and the
follow-ups included. Approving.
Three small things I found, none blocking. I'll carry them in my follow-up PR, so there is
nothing more for you to do here:
- The p-value floor has no test (my checklist's omission, not yours).
- esda's Moran row-standardises by default, so it gives a different I from morans_i on the
same W; the pointer will say transformation='b'. - lattice_weights(-2) quietly returns a 4x4 zero matrix; the follow-up rejects it.
Next from me: the shipped null/positive control and the reference-period baseline, built on
your lattice_weights.
Summary
A spatial early-warning-signal module, developed while building Hélios, a research/outreach project that replicates early-warning-signal methods on new domains and found itself needing this while doing so:
ewstools/spatial.py:morans_i(),morans_i_permutation_test(), and aSpatialEWSclass following the existingMultiTimeSeriesconventions (data->state->ews, transition-aware).ewstoolscovers the temporal branch of the critical-slowing-down literature thoroughly but had no equivalent for the spatial branch (Dakos et al., 2010) — confirmed absent by direct inspection ofcore.py/helpers.pybefore writing any code.(An earlier version of this PR also included
ewstools/pvalues.py, a correlated-p-value combiner. Removed per review: out of scope for this package, and it had its own bugs — see the review thread below for the full story.)Testing
tests/test_spatial.py: hand-computed Moran's I examples (verified independently againstesda.moran.Moran), known limiting cases (checkerboard, smooth gradient, constant field), the classical permutation-test propertyE[I] -> -1/(N-1), a p-value floor test, a NaN-handling warning test, and a negative control confirming that a strengthening spatial gradient raises Moran's I with no actual critical slowing down (see theSpatialEWSdocstring's trend-control recipe).CHANGELOG.mdupdated to match.Happy to adjust API shape, naming, or scope based on further maintainer feedback.
🤖 Generated with Claude Code