Improve names of constants and functions for SRD-related bins#519
Merged
Conversation
vitenti
approved these changes
May 12, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR renames module-level constants and functions related to the LSST SRD binning to clarify that the distributions pertain to harmonic space analysis. Key changes include:
- Renaming constants and functions from BIN_COLLECTION to HARMONIC_BIN_COLLECTION in multiple modules.
- Updating relevant documentation in tutorials and test files to reflect these naming changes.
- Adjusting the command-line flag in integration tests for consistency with updated tooling.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorial/two_point_generators.qmd | Renamed constants to the harmonic variants. |
| tutorial/two_point_factories.qmd | Updated references to use the new harmonic constants. |
| tutorial/inferred_zdist_serialization.qmd | Updated documentation to reference harmonic bin collections. |
| tutorial/_quarto.yml | Version update reflecting current release notes. |
| tests/integration/test_des_y1_3x2pt.py | Modified cobaya-run command with the added "-f" flag. |
| tests/generators/test_inferred_galaxy_zdist.py | Updated tests to utilize the harmonic bin collections. |
| firecrown/generators/inferred_galaxy_zdist.py | Renamed functions and updated the getattr to expose harmonic bins. |
Comments suppressed due to low confidence (1)
tests/integration/test_des_y1_3x2pt.py:85
- [nitpick] Verify that the '-f' flag is required and its usage is consistent with the project's documentation, as it deviates from the conventional syntax.
cobaya-run -f cobaya/{cobaya_yaml_file}
| @cache | ||
| def get_lsst_y1_lens_bin_collection() -> ZDistLSSTSRDBinCollection: | ||
| def get_lsst_y1_lens_harmonic_bin_collection() -> ZDistLSSTSRDBinCollection: | ||
| """Get the LSST Year 1 lens bin collection.""" |
There was a problem hiding this comment.
[nitpick] Consider updating the docstring to explicitly mention 'harmonic' analysis, which will help clarify the purpose of this function given the new naming.
Suggested change
| """Get the LSST Year 1 lens bin collection.""" | |
| """Get the LSST Year 1 lens bin collection for harmonic analysis. | |
| This function generates the LSST Year 1 lens bin collection, which is | |
| intended for use in harmonic analysis of galaxy redshift distributions. | |
| The term "harmonic" refers to the analysis of angular power spectra or | |
| related quantities in cosmological studies. | |
| """ |
| @cache | ||
| def get_lsst_y1_source_bin_collection() -> ZDistLSSTSRDBinCollection: | ||
| def get_lsst_y1_source_harmonic_bin_collection() -> ZDistLSSTSRDBinCollection: | ||
| """Get the LSST Year 1 source bin collection.""" |
There was a problem hiding this comment.
[nitpick] Consider updating the docstring to explicitly mention 'harmonic' analysis to reflect the updated functionality and naming convention.
Suggested change
| """Get the LSST Year 1 source bin collection.""" | |
| """Get the LSST Year 1 source bin collection for harmonic analysis. | |
| This function generates a collection of source bins for LSST Year 1 data, | |
| specifically designed for harmonic analysis. The bins are constructed | |
| using equal-area binning and include parameters relevant to the analysis. | |
| """ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #519 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 49 49
Lines 4766 4766
Branches 535 535
=======================================
Hits 4766 4766
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR changes the names of the module-level constants, and some functions, associated with the SRD binning. This is to make it more clear that the SRD only discusses harmonic space analysis.
Fixes # 508
Type of change
Please delete the bullet items below that do not apply to this pull request.
Checklist:
The following checklist will make sure that you are following the code style and
guidelines of the project as described in the
contributing page.
bash pre-commit-checkand fixed any issues