Skip to content

gh-1157: Add CosmologyWithInverseComovingDistance protocol#1164

Open
paddyroddy wants to merge 9 commits into
mainfrom
paddy/issue-1157
Open

gh-1157: Add CosmologyWithInverseComovingDistance protocol#1164
paddyroddy wants to merge 9 commits into
mainfrom
paddy/issue-1157

Conversation

@paddyroddy

@paddyroddy paddyroddy commented Jul 8, 2026

Copy link
Copy Markdown
Member

Description

Adds a CosmologyWithInverseComovingDistance protocol as inv_comoving_distance isn't used that often.

Closes: #1157

Changelog entry

Added: A CosmologyWithInverseComovingDistance protocol

Checks

  • Is your code passing linting?
  • Is your code passing tests?
  • Have you added additional tests (if required)?
  • Have you modified/extended the documentation (if required)?
  • Have you added a one-liner changelog entry above (if required)?

@paddyroddy
paddyroddy requested a review from Copilot July 8, 2026 11:00
@paddyroddy paddyroddy self-assigned this Jul 8, 2026
@paddyroddy paddyroddy added enhancement New feature or request science Science improvement or question needs-2-reviewers Could be considered "controversial" so worth a second pair of eyes labels Jul 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the typing surface around cosmology capabilities by introducing a dedicated CosmologyWithInverseComovingDistance protocol, so consumers can require inv_comoving_distance only where it’s actually needed.

Changes:

  • Remove HasInverseComovingDistance from the base Cosmology protocol and introduce CosmologyWithInverseComovingDistance.
  • Update glass.shells.distance_grid to require the new protocol.
  • Update test type annotations to reference the new protocol (with one inconsistency noted in review comments).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
glass/cosmology.py Splits inverse-comoving-distance capability into a dedicated protocol.
glass/shells.py Tightens distance_grid’s cosmology type requirement to the new protocol and updates its parameter docs.
tests/core/test_shells.py Adjusts test typing imports/annotations to align with the new protocol (needs a small follow-up for consistency).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread glass/shells.py
Comment thread tests/core/test_shells.py Outdated
@paddyroddy
paddyroddy requested review from connoraird and ntessore July 8, 2026 11:54

@connoraird connoraird left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surised there were no changes needed in the tests but what do I know

@paddyroddy

Copy link
Copy Markdown
Member Author

I'm surised there were no changes needed in the tests but what do I know

So I had previously modified the tests, but reverted these changes due to a Copilot review. inv_comoving_distance is only called in distance_grid and hence it only needs CosmologyWithInverseComovingDistance. test_distance_grid calls distance_grid, but the test itself would "work" with Cosmology only. This was the comment #1164 (comment).

@paddyroddy paddyroddy added the typing Static typing label Jul 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

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

Labels

enhancement New feature or request needs-2-reviewers Could be considered "controversial" so worth a second pair of eyes science Science improvement or question typing Static typing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove HasInverseComovingDistance from Cosmology protocol

3 participants