Conversation
access appears in both explore and model, causing memberships['access'] to silently resolve to 'model' (last-write wins). The __init__.py docstring and pysal.org docs place access under model only.
There was a problem hiding this comment.
Pull request overview
Removes the duplicate "access" entry from the explore tier in pysal.base.federation_hierarchy so the derived memberships mapping is unambiguous and aligns with the package-layer documentation.
Changes:
- Remove
"access"from theexplorelayer infederation_hierarchy(leaving it only undermodel). - Simplify the memberships consistency test now that
"access"no longer needs special-casing. - Add a regression test ensuring
_installed_version()returns"NA"for a nonexistent package.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pysal/base.py |
Removes "access" from explore in federation_hierarchy, making memberships["access"] deterministic. |
pysal/tests/test_base.py |
Updates membership-layer assertions and adds a nonexistent-package test for _installed_version(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "pointpats", | ||
| "inequality", | ||
| "spaghetti", | ||
| "access", | ||
| "momepy", |
There was a problem hiding this comment.
PR metadata/linked issue #1424 describes an ImportError/__all__ export problem for federation_hierarchy/memberships, but this PR only changes the federation_hierarchy contents (removing access from explore) and related tests. If the intent is to close #1424, the exports in pysal/__init__.py (or the issue reference/PR description) likely needs to be updated so they align with the actual fix being made here.
| # Note: test for nonexistent package is omitted because the current | ||
| # implementation has a bug (NameError instead of returning 'NA'). | ||
| # This will be fixed in a separate PR. | ||
| def test_installed_version_for_nonexistent_package(self): |
There was a problem hiding this comment.
For consistency with the rest of the tests in this class/file (most test methods have a short docstring), consider adding a brief docstring to this newly added test as well.
| def test_installed_version_for_nonexistent_package(self): | |
| def test_installed_version_for_nonexistent_package(self): | |
| """Test version detection for a nonexistent package.""" |
fixes #1424.
accesswas listed in bothexploreandmodeltiers offederation_hierarchy, causing Python's dict assignment to silently discard theexploreentry somemberships["access"]always resolved to"model"regardless of iteration order. The duplicateexploreentry is removed, aligning the data structure with the__init__.pydocstring and pysal.org which placeaccessexclusively undermodel.