Depletion changes #125
Depletion changes #125
Conversation
WalkthroughThis PR updates Cloudy grid generation: constructs depletion_model objects for Abundances, coerces abundance_scalings entries to numeric when possible, ensures continuum arrays exist (zero-filled when not calculated), adjusts CLI/defaults and axis metadata, refines list-file index loading/selection, and edits/adds multiple Cloudy YAML parameter files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateInput as create_cloudy_input_grid.py
participant DeplModels as depletion_models
participant Abundances
User->>CreateInput: call with config (depletion name/scale, abundance_scalings)
CreateInput->>CreateInput: coerce abundance_scalings values to float when possible
CreateInput->>DeplModels: lookup depletion_model by name
DeplModels-->>CreateInput: depletion_model object
CreateInput->>CreateInput: set depletion_scale (default 1.0)
CreateInput->>Abundances: instantiate with depletion_model object and scalings
Abundances-->>CreateInput: Abundances instance
CreateInput-->>User: return Abundances
sequenceDiagram
participant add_lines
participant continuum_list as continuum_quantities
participant output as output_arrays
add_lines->>continuum_list: define continuum quantity list
alt calculate_continuum == True
add_lines->>output: compute continuum arrays and store
else calculate_continuum == False
add_lines->>output: allocate zero-filled continuum arrays
end
add_lines->>output: apply units to all continuum_quantities unconditionally
add_lines-->>output: finalized arrays
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-14T12:59:09.389ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ydrogen density to 10^2.5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/synthesizer_grids/cloudy/params/test_suite/CO.yaml (1)
1-3: Clarify the structure of abundance_scalings.The nested structure has
carbon:on line 2 with no value, followed bycarbon_to_oxygen:with values. This creates an emptyabundance_scalings.carbonentry whileabundance_scalings.carbon_to_oxygenholds the actual data.Consider flattening the structure if the
carbon:key serves no purpose:abundance_scalings: - carbon: carbon_to_oxygen: [-1.5, -1.0, -0.34, 0.0] # scaling for Carbon, either float relative to Solar or string defining the in-built function to useIf the nested structure is intentional for organizational purposes, please add a comment explaining why.
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py (2)
73-88: Add error handling for invalid depletion model names.The code uses
getattr(depletion_models, parameters["depletion_model"])which will raiseAttributeErrorif the specified depletion model doesn't exist. Consider adding error handling to provide a more helpful error message:depletion_model = getattr( depletion_models, parameters["depletion_model"] - )(scale=depletion_scale) + ) + if depletion_model is None: + raise ValueError( + f"Depletion model '{parameters['depletion_model']}' not found. " + f"Available models: {dir(depletion_models)}" + ) + depletion_model = depletion_model(scale=depletion_scale)Or wrap in try-except:
+ try: depletion_model = getattr( depletion_models, parameters["depletion_model"] )(scale=depletion_scale) + except AttributeError: + raise ValueError( + f"Unknown depletion model: '{parameters['depletion_model']}'" + )
90-93: Consider using logging instead of print statements.The debug print statements are helpful during development, but consider using Python's logging module for better control over output verbosity in production:
import logging logger = logging.getLogger(__name__) logger.debug(f"metallicities: {parameters['metallicities']}") logger.debug(f"reference_abundance: {parameters['reference_abundance']}") logger.debug(f"alpha_enhancement: {parameters['alpha_enhancement']}") logger.debug(f"abundance_scalings: {parameters['abundance_scalings']}")This allows users to control verbosity via logging configuration without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py(3 hunks)src/synthesizer_grids/cloudy/create_synthesizer_grid.py(5 hunks)src/synthesizer_grids/cloudy/params/c23.01-blr-test.yaml(1 hunks)src/synthesizer_grids/cloudy/params/c23.01-nlr-test.yaml(1 hunks)src/synthesizer_grids/cloudy/params/c23.01-nlr.yaml(1 hunks)src/synthesizer_grids/cloudy/params/c23.01-sps-UnH.yaml(1 hunks)src/synthesizer_grids/cloudy/params/c23.01-sps-grid.yaml(1 hunks)src/synthesizer_grids/cloudy/params/c23.01-sps.yaml(1 hunks)src/synthesizer_grids/cloudy/params/test_suite/CO.yaml(1 hunks)src/synthesizer_grids/cloudy/params/test_suite/NO.yaml(1 hunks)src/synthesizer_grids/cloudy/params/test_suite/fixed_ionisation_parameter.yaml(1 hunks)src/synthesizer_grids/cloudy/run_cloudy.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: christopherlovell
Repo: synthesizer-project/grid-generation PR: 132
File: src/synthesizer_grids/cloudy/submission_scripts.py:434-463
Timestamp: 2025-11-14T12:59:09.389Z
Learning: In the cosma7_sobol_onthefly function in src/synthesizer_grids/cloudy/submission_scripts.py, work directories are intentionally left behind when Cloudy runs fail or 0.cont is missing, to aid debugging. Only successful Cloudy runs have their temporary work directories cleaned up.
📚 Learning: 2025-11-14T12:59:09.389Z
Learnt from: christopherlovell
Repo: synthesizer-project/grid-generation PR: 132
File: src/synthesizer_grids/cloudy/submission_scripts.py:434-463
Timestamp: 2025-11-14T12:59:09.389Z
Learning: In the cosma7_sobol_onthefly function in src/synthesizer_grids/cloudy/submission_scripts.py, work directories are intentionally left behind when Cloudy runs fail or 0.cont is missing, to aid debugging. Only successful Cloudy runs have their temporary work directories cleaned up.
Applied to files:
src/synthesizer_grids/cloudy/params/c23.01-sps-grid.yamlsrc/synthesizer_grids/cloudy/run_cloudy.pysrc/synthesizer_grids/cloudy/create_synthesizer_grid.py
🔇 Additional comments (14)
src/synthesizer_grids/cloudy/params/c23.01-sps-grid.yaml (1)
43-43: Continuum output enabled.The change aligns with broader continuum handling refinements noted in the PR. No concerns.
src/synthesizer_grids/cloudy/params/c23.01-sps.yaml (1)
28-29: Verify motivation for hydrogen density reduction.The density decreased from 1.0e+3 to 3.16e+2 cm⁻³ (~3× reduction). The change lacks a rationale comment. Please confirm this is intentional and aligns with the PR's physical/numerical objectives.
src/synthesizer_grids/cloudy/params/c23.01-sps-UnH.yaml (1)
1-45: New test suite properly structured.The new c23.01-sps-UnH.yaml introduces a 4×4 ionization-parameter/density grid with appropriate parameter ranges and conventions. Configuration aligns with existing SPS parameter files.
src/synthesizer_grids/cloudy/params/test_suite/fixed_ionisation_parameter.yaml (1)
2-2: Clarify geometry change motivation.The test file changed from
planeparalleltospheregeometry. Given the test name emphasizes "fixed ionisation parameter," please confirm this geometry modification is intentional and explain the physical rationale.src/synthesizer_grids/cloudy/params/c23.01-nlr-test.yaml (1)
25-25: Verify ionisation parameter subset rationale.The test file restricts ionisation parameters to [0.001, 0.01], removing the 0.1 value. Confirm this subset adequately covers the parameter space for testing while balancing runtime/coverage tradeoffs.
src/synthesizer_grids/cloudy/params/c23.01-blr-test.yaml (1)
29-29: Clarify stopping column density adjustment.The test bounds shifted from [22, 23] to [23, 25], increasing both minimum and span. Please confirm this reflects an intentional refinement to BLR stopping criteria and document the physical rationale.
src/synthesizer_grids/cloudy/params/c23.01-nlr.yaml (1)
25-25: Ionisation parameter grid appropriately expanded.The NLR configuration extends ionization parameters to [0.0001, 0.001, 0.01, 0.1], adding lower-U coverage. This aligns with enhanced abundance/depletion modeling in the PR and is consistent with the parallel sps-UnH grid.
src/synthesizer_grids/cloudy/run_cloudy.py (2)
113-123: LGTM: Improved control flow checks.The change from
!= Falsetois not Noneis more explicit and idiomatic. This correctly handles the case where these arguments are either provided (with a value) or not provided (defaulting to None).
157-159: LGTM: Helpful debug output.The debug print statements provide visibility into the computed index pairs before execution, which is useful for troubleshooting and verifying the correct indices are being processed.
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py (2)
96-102: LGTM: Depletion model passed correctly.The change to pass
depletion_modelas an object (instead of passingdepletion_modelanddepletion_scaleseparately) aligns with the PR objectives to match recent changes in thesynthesizerproject.
143-143: LGTM: Typo fixed.Good catch fixing "regnoised" to "recognised".
src/synthesizer_grids/cloudy/create_synthesizer_grid.py (3)
41-42: LGTM: Abundance scaling axes properly configured.The new
abundance_scalings.nitrogen_to_oxygenandabundance_scalings.carbon_to_oxygenaxes are consistently added toaxes_units,log_on_read_dict, andpluralisation_of_axesdictionaries, ensuring proper handling throughout the grid creation process.Also applies to: 55-56, 69-74
671-708: LGTM: Continuum arrays now always initialized.The restructured logic ensures continuum quantity arrays (
transmitted,incident,nebular_continuum,total_continuum) are always created, even whencalculate_continuumis False. This prevents potentialKeyErrororAttributeErrorexceptions in downstream code that expects these arrays to exist.When spectra aren't provided, the arrays are zero-filled, which is a sensible default. The tradeoff is slightly increased memory usage, but this is acceptable for improved consistency and robustness.
795-796: LGTM: Units consistently applied.Since continuum quantity arrays are now always created (lines 671-708), it's correct to always apply units to them, regardless of whether they contain calculated values or zeros.
| abundance_scalings: | ||
| nitrogen: | ||
| nitrogen_to_oxygen: [-2.0, -1.5, -1.03, -0.5] No newline at end of file |
There was a problem hiding this comment.
Fix malformed abundance_scalings structure.
Line 2 contains an empty nitrogen: key with no value, followed by nitrogen_to_oxygen: at the same indentation level. This produces an unintended YAML structure where nitrogen becomes null. Correct this to:
abundance_scalings:
nitrogen_to_oxygen: [-2.0, -1.5, -1.03, -0.5]Remove the empty nitrogen: line.
🤖 Prompt for AI Agents
In src/synthesizer_grids/cloudy/params/test_suite/NO.yaml around lines 1 to 3,
the YAML has an empty `nitrogen:` key which makes `nitrogen` null and
misstructures `abundance_scalings`; remove the empty `nitrogen:` line and ensure
`nitrogen_to_oxygen: [-2.0, -1.5, -1.03, -0.5]` remains indented under
`abundance_scalings` so the file becomes a single mapping with only
`nitrogen_to_oxygen`.
| # convert to synthesizer standard | ||
| parameters["abundance_scalings"][kk] = v | ||
|
|
||
| if v: |
There was a problem hiding this comment.
Be more specific here than a truthy check. Are you simply checking if it's not None?
| else: | ||
| depletion_model = None | ||
|
|
||
| print(parameters["metallicities"]) |
There was a problem hiding this comment.
More context needed for prints
| if v: | ||
| # The value (v) here could be a bona fide string or number, | ||
| # try to convert to a | ||
| try: |
There was a problem hiding this comment.
Use isinstance for specificity, not error handling.
| "transmitted", | ||
| "incident", | ||
| "nebular_continuum", | ||
| "total_continuum", |
There was a problem hiding this comment.
What is this? We don't use this for anything do we? Might be worth saving that space on disk.
| incident_indices = incident_indices.flatten() | ||
| photoionisation_indices = photoionisation_indices.flatten() | ||
|
|
||
| print(incident_indices) |
There was a problem hiding this comment.
Again, print context. These bare prints are meaningless to anyone that hasn't written the code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py (1)
90-93: Add context labels to debug prints.These print statements lack context about what's being printed. Add descriptive labels to make the output clear.
Apply this diff to add labels:
- print(parameters["metallicities"]) - print(parameters["reference_abundance"]) - print(parameters["alpha_enhancement"]) - print(parameters["abundance_scalings"]) + print(f"Metallicities: {parameters['metallicities']}") + print(f"Reference abundance: {parameters['reference_abundance']}") + print(f"Alpha enhancement: {parameters['alpha_enhancement']}") + print(f"Abundance scalings: {parameters['abundance_scalings']}")
🧹 Nitpick comments (1)
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py (1)
73-88: Add error handling for invalid depletion model names.The
getattrcall will raise anAttributeErrorifparameters["depletion_model"]references a model that doesn't exist indepletion_models. Consider adding error handling to provide a clearer error message.Apply this diff to add validation:
# Initialise depletion model if "depletion_model" in parameters.keys(): if parameters["depletion_model"] is None: depletion_model = None else: if "depletion_scale" not in parameters.keys(): depletion_scale = 1.0 else: depletion_scale = parameters["depletion_scale"] - depletion_model = getattr( - depletion_models, parameters["depletion_model"] - )(scale=depletion_scale) + try: + depletion_model = getattr( + depletion_models, parameters["depletion_model"] + )(scale=depletion_scale) + except AttributeError: + raise ValueError( + f"Unknown depletion model: {parameters['depletion_model']}" + ) else: depletion_model = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: christopherlovell
Repo: synthesizer-project/grid-generation PR: 132
File: src/synthesizer_grids/cloudy/submission_scripts.py:434-463
Timestamp: 2025-11-14T12:59:09.389Z
Learning: In the cosma7_sobol_onthefly function in src/synthesizer_grids/cloudy/submission_scripts.py, work directories are intentionally left behind when Cloudy runs fail or 0.cont is missing, to aid debugging. Only successful Cloudy runs have their temporary work directories cleaned up.
🔇 Additional comments (4)
src/synthesizer_grids/cloudy/create_cloudy_input_grid.py (4)
14-14: LGTM: Import addition supports new depletion model handling.The import of
depletion_modelsis correctly added to support the new depletion model initialization logic introduced later in the file.
62-71: Zero-value handling fixed; try/except is appropriate here.The change from
if v:toif v is not None:correctly addresses the previous issue where zero values would be incorrectly skipped.Regarding the previous comment about using
isinstanceinstead of try/except: the try/except approach is appropriate here because the valuevcould be a string representation of a number (e.g.,"1.5"). Theisinstancecheck cannot determine if a string is numeric—you must attempt the conversion. This follows Python's EAFP (Easier to Ask for Forgiveness than Permission) idiom, which is idiomatic for type coercion.
143-143: LGTM: Typo fix.Good catch fixing "regnoised" to "recognised" in the comment.
96-102: No issues found—API change is correct.The code correctly implements the synthesizer library's API. The
Abundancesconstructor acceptsdepletion_modelas a single parameter that can be either a string (e.g., "CloudyClassic") or a depletion_models instance (e.g.,Gutkin2016(scale=0.8)). The change from separatedepletion_modelanddepletion_scaleparameters to a unifieddepletion_modelobject aligns with the current synthesizer API design.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/synthesizer_grids/cloudy/README.md (1)
62-72: Fix spelling/grammar and correct the--include-spectraflag in the README.In this section:
acheived→achieved.- “if this is desired it necessary to add” → “if this is desired it is necessary to add”.
- The flag in the example and text should match the CLI, which defines
--include-spectra(hyphen), not--include_spectra(underscore), otherwise users copying the command will get an error.Suggested patch:
-Finally, we need to create a new `synthesizer` grid object containing the cloudy computed lines and spectra. This is acheived by running `create_synthesizer_grid.py`. By default spectra are not extracted; if this is desired it necessary to add the `include_spectra` argument as below. +Finally, we need to create a new `synthesizer` grid object containing the cloudy computed lines and spectra. This is achieved by running `create_synthesizer_grid.py`. By default spectra are not extracted; if this is desired it is necessary to add the `--include-spectra` argument as below. @@ - --cloudy-paramfile-extra=test_suite/reference_ionisation_parameter - --include_spectra \ + --cloudy-paramfile-extra=test_suite/reference_ionisation_parameter + --include-spectra \src/synthesizer_grids/cloudy/create_synthesizer_grid.py (1)
845-852:--norm-by-QCLI flag is currently ignored; wire it intoadd_spectra.The CLI adds a
--norm-by-Q / -Qoption:parser.add_argument( "--norm-by-Q", "-Q", default=True, ... )but the call to
add_spectrahardcodesnorm_by_q=True, so the user’s choice has no effect:lam, spectra = add_spectra( ..., spec_names=("incident", "transmitted", "nebular", "linecont"), norm_by_q=True, )This makes
--norm-by-Qeffectively dead.Minimal fix (assuming
Parsergives you a usable boolean):- lam, spectra = add_spectra( + lam, spectra = add_spectra( new_grid, new_grid_name, cloudy_dir, incident_index_list, photoionisation_index_list, new_shape, spec_names=("incident", "transmitted", "nebular", "linecont"), - norm_by_q=True, + norm_by_q=args.norm_by_Q, )If
args.norm_by_Qisn’t already a proper bool, you may also want to adjust the argument definition (e.g. using a store_true/store_false style or explicit type conversion in yourParser) so thatnorm_by_qis reliably boolean.Also applies to: 1021-1032
♻️ Duplicate comments (1)
src/synthesizer_grids/cloudy/create_synthesizer_grid.py (1)
835-842:--include-spectradefault False: behavior is clear but remains a breaking change.Setting
default=Falsehere matches the updated README text that spectra are now opt-in. From a code perspective this is fine, but it does invert the previous default and will change behavior for any workflows that didn’t explicitly pass the flag.As long as this is called out in release notes / migration docs (which a previous review already requested), the change itself looks good.
🧹 Nitpick comments (1)
src/synthesizer_grids/cloudy/create_synthesizer_grid.py (1)
667-707: Continuum handling inadd_linesis now robust without spectra; update comments / minor simplification.Behavior-wise this looks solid:
continuum_quantitiesis now defined once and used consistently.- When
spectra is None, you still allocate continuum arrays and fill them with zeros, avoiding interpolation on alam=Nonegrid and ensuring downstream consumers always see the same keys.- Units are applied to all continuum quantities regardless of whether they were interpolated or zero-initialized, which is fine (zero-valued arrays with units).
Two small polish points:
- The comment above
continuum_quantities/lines["luminosity"]still says “only save continuum values if spectra are provided,” but you now always write the continuum datasets (zeros whencalculate_continuumis False). It would be clearer to reword this to reflect the new behavior.- In the failure branch you still conditionally zero the continuum arrays only when
calculate_continuumis True; since the non-spectra path pre-zeros them, that innerif calculate_continuum:block is now redundant and could be dropped for simplicity.Also applies to: 795-797
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/synthesizer_grids/cloudy/README.md(2 hunks)src/synthesizer_grids/cloudy/create_synthesizer_grid.py(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: christopherlovell
Repo: synthesizer-project/grid-generation PR: 132
File: src/synthesizer_grids/cloudy/submission_scripts.py:434-463
Timestamp: 2025-11-14T12:59:09.389Z
Learning: In the cosma7_sobol_onthefly function in src/synthesizer_grids/cloudy/submission_scripts.py, work directories are intentionally left behind when Cloudy runs fail or 0.cont is missing, to aid debugging. Only successful Cloudy runs have their temporary work directories cleaned up.
📚 Learning: 2025-11-14T12:59:09.389Z
Learnt from: christopherlovell
Repo: synthesizer-project/grid-generation PR: 132
File: src/synthesizer_grids/cloudy/submission_scripts.py:434-463
Timestamp: 2025-11-14T12:59:09.389Z
Learning: In the cosma7_sobol_onthefly function in src/synthesizer_grids/cloudy/submission_scripts.py, work directories are intentionally left behind when Cloudy runs fail or 0.cont is missing, to aid debugging. Only successful Cloudy runs have their temporary work directories cleaned up.
Applied to files:
src/synthesizer_grids/cloudy/README.mdsrc/synthesizer_grids/cloudy/create_synthesizer_grid.py
🪛 LanguageTool
src/synthesizer_grids/cloudy/README.md
[grammar] ~62-~62: Ensure spelling is correct
Context: ...udy computed lines and spectra. This is acheived by running create_synthesizer_grid.py...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
src/synthesizer_grids/cloudy/create_synthesizer_grid.py (1)
69-75: Axes pluralisation for abundance_scalings looks consistent.The new pluralisation entries for
abundance_scalings.nitrogen_to_oxygenandabundance_scalings.carbon_to_oxygenkeeppluralisation_of_axes,axes_units, andlog_on_read_dictaligned and avoid the ValueError path when these axes appear. Mapping to the same string is fine given the existing naming scheme.
Changes how depletion is handled aligning with recent changes to changes to
synthesizer.Also updates a few of the default parameter files.
Issue Type
Checklist
Summary by CodeRabbit