FSPS wavelength offset #137
Conversation
…his PR. Fix notebook cells.
TobiBu
left a comment
TobiBu
left a comment
There was a problem hiding this comment.
once the one fix is done, we can merge.
rubix/spectra/ssp/fsps_grid.py
Outdated
| # Adjust the wavelength grid to the bin centers: | ||
| # _wave[0] and _wave[1] are different by 3, to center, we have to shift half way, so subtract 1.5 A | ||
| # to test that the centering is correct, we can look at the position of the Halpha line at 6563 A | ||
| ssp_wave_centered = ssp_wave - 1.5 |
There was a problem hiding this comment.
i think it makes more sense to to use offset = (_wave[1] - _wave[0]) / 2.
and not a fixed 1.5 as this value might depend on the exact wavelength grid used.
There was a problem hiding this comment.
Yes, very good point! I just changed it in the code
for more information, see https://pre-commit.ci
|
How is the progress on this @anschaible? |
for more information, see https://pre-commit.ci
Resolve outdated branch, get compare_ssp up to date with the main branch
for more information, see https://pre-commit.ci
@TobiBu , I changed the code regarding to your suggestions. Also I fixed the pytests that they now run and cleaned a bit the branch. If we get two reviews to the branch, we can merge it to the main |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an incorrect wavelength grid centering in the FSPS template by shifting the wavelength array to bin centers and updating tests and documentation accordingly. It also adds a new telescope configuration, ensures numeric constants are cast properly, and clarifies configuration comments.
- Center the FSPS wavelength grid on bin centers and update the grid creation
- Update tests to expect the shifted wavelength values
- Add a new MUSE_NFM instrument and clarify config behavior for “source”
- Cast constant lookups to floats in IFU conversion
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ssp_fsps.py | Change expected wavelength to include the 50 Å offset |
| rubix/spectra/ssp/fsps_grid.py | Compute and apply dynamic half‐bin offset on wavelength |
| rubix/telescope/telescopes.yaml | Add new MUSE_NFM instrument definition |
| rubix/spectra/ifu.py | Cast CONSTANTS lookup to float for consistency |
| rubix/config/rubix_config.yml | Expand comments on source options |
| notebooks/... | Remove obsolete pipeline notebook; add comparison notebooks |
Comments suppressed due to low confidence (3)
rubix/spectra/ssp/fsps_grid.py:120
- Consider adding a unit test that verifies the offset calculation for non-uniform or edge-case wavelength grids to ensure centering logic remains correct if SP returns unexpected spacing.
offset = (_wave[1] - _wave[0]) / 2.0
rubix/spectra/ifu.py:34
- [nitpick] The name
CONSTis very generic; consider renaming to something more descriptive (e.g.,LSOL_TO_ERG_PER_MPC2) to clarify what this constant represents.
CONST = float(CONSTANTS.get("LSOL_TO_ERG")) / float(CONSTANTS.get("MPC_TO_CM")) ** 2
rubix/config/rubix_config.yml:220
- [nitpick] The commented-out
"rerun_from_scratch"option is indented just like a config entry, which could confuse readers; move it into a dedicated comment block or list the allowed values explicitly undersource.
# "rerun_from_scratch" # note: this is just meant for the case in which you really want to rerun your template library.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci
|
I approved. |
The FSPS template is offset in the wavelength (see position of Halpha line), this pull request fixes this issue.