Emuv3 - #87
Open
DanielaBreitman wants to merge 7 commits into
Open
Conversation
Reviewer's GuideAdds Emu v3 and RadioEMU support, extends likelihoods to work with new emulator outputs and pspec_likelihood, and introduces Nautilus and richer UltraNest/MultiNest options in the MCMC driver along with assorted logging and data-handling tweaks. Sequence diagram for Nautilus sampler integration in run_mcmcsequenceDiagram
participant User
participant run_mcmc
participant NautilusSampler as nautilus.Sampler
participant Chain as LikelihoodComputationChain
participant Core as CoreModules
participant Lk as LikelihoodModules
User->>run_mcmc: run_mcmc(core_modules, likelihood_modules, params, use_nautilus=True)
run_mcmc->>Chain: build_computation_chain(core_modules, likelihood_modules, params)
run_mcmc->>Chain: chain.setup()
run_mcmc->>NautilusSampler: Sampler(prior, likelihood, n_dim, n_live, ...)
Note over run_mcmc,NautilusSampler: likelihood is a wrapper calling chain
NautilusSampler->>Chain: likelihood(p)
Chain->>Core: build_model_data(Params)
Core-->>Chain: context with emulator outputs
Chain->>Lk: computeLikelihoods(context)
Lk-->>Chain: total loglike
Chain-->>NautilusSampler: loglike
alt post_only is False
User->>NautilusSampler: run(verbose, discard_exploration)
NautilusSampler-->>User: posterior(equal_weight, equal_weight_boost)
else post_only is True
NautilusSampler-->>User: posterior(equal_weight, equal_weight_boost)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The new LikelihoodNeutralFractionTwoSided class is defined twice with different implementations, which will cause one definition to overwrite the other; consolidate into a single class or clearly differentiate the names.
- In LikelihoodArcade.init you call get_T_arcade(redshift) as a free function, but it is defined as an instance method (self.get_T_arcade); this will raise an error and should be changed to use the bound method.
- Core21cmEMU/CoreRadioEMU parameter handling includes branches that reference variables like ap and astro_param_defaults that are not always initialized (e.g., ap in the dict branch and astro_param_defaults in CoreRadioEMU), which can lead to runtime errors; consider simplifying and centralizing the construction of the astro_params dict to avoid these cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new LikelihoodNeutralFractionTwoSided class is defined twice with different implementations, which will cause one definition to overwrite the other; consolidate into a single class or clearly differentiate the names.
- In LikelihoodArcade.__init__ you call get_T_arcade(redshift) as a free function, but it is defined as an instance method (self.get_T_arcade); this will raise an error and should be changed to use the bound method.
- Core21cmEMU/CoreRadioEMU parameter handling includes branches that reference variables like ap and astro_param_defaults that are not always initialized (e.g., ap in the dict branch and astro_param_defaults in CoreRadioEMU), which can lead to runtime errors; consider simplifying and centralizing the construction of the astro_params dict to avoid these cases.
## Individual Comments
### Comment 1
<location path="src/py21cmmc/likelihood.py" line_range="923-924" />
<code_context>
+ self.redshift = redshift
+ # If redshift is provided, evaluate the arcade model now
+ # Otherwise will be evaluated every time on emulator zs
+ if redshift is not None:
+ self.arcade = get_T_arcade(redshift)
+
+ def get_T_arcade(self,z):
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `get_T_arcade` without `self` will raise a NameError.
Here `get_T_arcade` is used as a free function, but it’s defined as an instance method. Consider calling `self.get_T_arcade(redshift)` instead, or refactor `get_T_arcade` into a `@staticmethod` or module-level function and update all call sites.
</issue_to_address>
### Comment 2
<location path="src/py21cmmc/likelihood.py" line_range="233" />
<code_context>
try:
from classy import Class
</code_context>
<issue_to_address>
**issue (bug_risk):** Exception handling can leave `model_spline`/`err_spline` undefined and prints directly to stdout.
In `LikelihoodNeutralFraction.computeLikelihood`, if `InterpolatedUnivariateSpline` raises, the `except` block only prints `(xHI.shape, n)` and continues. This can leave `model_spline`/`err_spline` undefined and cause an `UnboundLocalError` later, and direct `print` calls inside a library API bypass proper logging. Please either re‑raise with a clearer error or implement a safe fallback path, and use the project logger (or remove the print) instead of `print`.
</issue_to_address>
### Comment 3
<location path="src/py21cmmc/likelihood.py" line_range="1567-1576" />
<code_context>
+class LikelihoodNeutralFractionTwoSided(LikelihoodNeutralFraction):
</code_context>
<issue_to_address>
**issue (bug_risk):** This new `LikelihoodNeutralFractionTwoSided` class duplicates the existing one below and silently gets overridden.
This makes the new implementation dead code and can mislead future maintainers. Please either remove the old definition, rename one of the classes, or consolidate the intended behaviour into a single class.
</issue_to_address>
### Comment 4
<location path="src/py21cmmc/core.py" line_range="1340-1341" />
<code_context>
+ try:
+ values.append(astro_params[k])
+ except KeyError:
+ if k == 'L_X_MINI':
+ values.append(ap['L_X'])
+ else:
+ values.append(self.astro_param_defaults[k])
</code_context>
<issue_to_address>
**issue (bug_risk):** Use of `ap` before it is defined when filling missing astro parameters.
In `Core21cmEMU.build_model_data`, inside the `isinstance(astro_params, dict)` branch, the fallback for missing `L_X_MINI` references `ap['L_X']`, but `ap` is only defined later in the non‑dict path. This will raise a `NameError` when `L_X_MINI` is absent. Consider either constructing `ap` from `astro_params` earlier in the function or using `astro_params['L_X']` directly for this fallback.
</issue_to_address>
### Comment 5
<location path="src/py21cmmc/core.py" line_range="1343" />
<code_context>
+ if k == 'L_X_MINI':
+ values.append(ap['L_X'])
+ else:
+ values.append(self.astro_param_defaults[k])
+ elif isinstance(astro_params, p21.AstroParams):
+ values = astro_params.defining_dict.values
</code_context>
<issue_to_address>
**issue (bug_risk):** `CoreRadioEMU` references `self.astro_param_defaults` which is never defined in the class.
`build_model_data` falls back to `self.astro_param_defaults[k]`, but `CoreRadioEMU.__init__` never sets `self.astro_param_defaults` (unlike `Core21cmEMU`). This will raise an `AttributeError` when a missing key is encountered. Please either define `astro_param_defaults` in `CoreRadioEMU.__init__` (e.g., via an argument) or inline the required defaults at the call site.
</issue_to_address>
### Comment 6
<location path="src/py21cmmc/mcmc.py" line_range="288-289" />
<code_context>
multimodal = mcmc_options.get("multimodal", True)
write_output = mcmc_options.get("write_output", True)
datadir = datadir + "/MultiNest/"
+ verbose = mcmc_options.get("vectorize", True)
try:
from pymultinest import run
</code_context>
<issue_to_address>
**issue (bug_risk):** MultiNest `verbose` flag is incorrectly tied to the `vectorize` option.
`verbose` is currently derived from `mcmc_options.get("vectorize", True)`, which makes verbosity depend on an unrelated option and is almost certainly a typo (likely intended to be `mcmc_options.get("verbose", True)`). This can cause confusing logging behaviour. Please introduce/use a dedicated `verbose` option instead.
</issue_to_address>
### Comment 7
<location path="src/py21cmmc/mcmc.py" line_range="549-558" />
<code_context>
-
+
def likelihood(p):
if vectorized:
return chain.computeLikelihoods(
chain.build_model_data(
</code_context>
<issue_to_address>
**issue (bug_risk):** Nautilus likelihood assumes `p` is a dict, but Nautilus passes arrays; vectorized path will likely break.
In the Nautilus vectorized path, `likelihood` assumes `p` is a mapping (`zip(p.keys(), p.values())`), but Nautilus passes NumPy arrays by default. This will raise at runtime. You likely need a fixed parameter ordering (e.g., from `params.keys`) and to map array entries to `Params`, as done in the MultiNest/UltraNest paths. If you instead rely on `pass_dict`, the vectorized handling should be conditioned on that mode and clearly separated/documented.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+923
to
+924
| if redshift is not None: | ||
| self.arcade = get_T_arcade(redshift) |
There was a problem hiding this comment.
issue (bug_risk): Calling get_T_arcade without self will raise a NameError.
Here get_T_arcade is used as a free function, but it’s defined as an instance method. Consider calling self.get_T_arcade(redshift) instead, or refactor get_T_arcade into a @staticmethod or module-level function and update all call sites.
| ) | ||
|
|
||
| else: | ||
| try: |
There was a problem hiding this comment.
issue (bug_risk): Exception handling can leave model_spline/err_spline undefined and prints directly to stdout.
In LikelihoodNeutralFraction.computeLikelihood, if InterpolatedUnivariateSpline raises, the except block only prints (xHI.shape, n) and continues. This can leave model_spline/err_spline undefined and cause an UnboundLocalError later, and direct print calls inside a library API bypass proper logging. Please either re‑raise with a clearer error or implement a safe fallback path, and use the project logger (or remove the print) instead of print.
Comment on lines
+1567
to
+1576
| class LikelihoodNeutralFractionTwoSided(LikelihoodNeutralFraction): | ||
| """ | ||
| A likelihood based on the measured neutral fraction at a range of redshifts. | ||
|
|
||
| The log-likelihood statistic is a simple chi^2. | ||
| """ | ||
|
|
||
| required_cores = ( | ||
| ( | ||
| core.CoreLightConeModule, |
There was a problem hiding this comment.
issue (bug_risk): This new LikelihoodNeutralFractionTwoSided class duplicates the existing one below and silently gets overridden.
This makes the new implementation dead code and can mislead future maintainers. Please either remove the old definition, rename one of the classes, or consolidate the intended behaviour into a single class.
Comment on lines
+1340
to
+1341
| if k == 'L_X_MINI': | ||
| values.append(ap['L_X']) |
There was a problem hiding this comment.
issue (bug_risk): Use of ap before it is defined when filling missing astro parameters.
In Core21cmEMU.build_model_data, inside the isinstance(astro_params, dict) branch, the fallback for missing L_X_MINI references ap['L_X'], but ap is only defined later in the non‑dict path. This will raise a NameError when L_X_MINI is absent. Consider either constructing ap from astro_params earlier in the function or using astro_params['L_X'] directly for this fallback.
| if k == 'L_X_MINI': | ||
| values.append(ap['L_X']) | ||
| else: | ||
| values.append(self.astro_param_defaults[k]) |
There was a problem hiding this comment.
issue (bug_risk): CoreRadioEMU references self.astro_param_defaults which is never defined in the class.
build_model_data falls back to self.astro_param_defaults[k], but CoreRadioEMU.__init__ never sets self.astro_param_defaults (unlike Core21cmEMU). This will raise an AttributeError when a missing key is encountered. Please either define astro_param_defaults in CoreRadioEMU.__init__ (e.g., via an argument) or inline the required defaults at the call site.
Comment on lines
288
to
+289
| datadir = datadir + "/MultiNest/" | ||
| verbose = mcmc_options.get("vectorize", True) |
There was a problem hiding this comment.
issue (bug_risk): MultiNest verbose flag is incorrectly tied to the vectorize option.
verbose is currently derived from mcmc_options.get("vectorize", True), which makes verbosity depend on an unrelated option and is almost certainly a typo (likely intended to be mcmc_options.get("verbose", True)). This can cause confusing logging behaviour. Please introduce/use a dedicated verbose option instead.
Comment on lines
549
to
558
| if vectorized: | ||
| return chain.computeLikelihoods( | ||
| chain.build_model_data( | ||
| Params(*[(k, v) for k, v in zip(params.keys, p.T)]) | ||
| ) | ||
| ) | ||
| ).squeeze() | ||
| else: | ||
| try: | ||
| return chain.computeLikelihoods( | ||
| chain.build_model_data( |
There was a problem hiding this comment.
issue (bug_risk): Nautilus likelihood assumes p is a dict, but Nautilus passes arrays; vectorized path will likely break.
In the Nautilus vectorized path, likelihood assumes p is a mapping (zip(p.keys(), p.values())), but Nautilus passes NumPy arrays by default. This will raise at runtime. You likely need a fixed parameter ordering (e.g., from params.keys) and to map array entries to Params, as done in the MultiNest/UltraNest paths. If you instead rely on pass_dict, the vectorized handling should be conditioned on that mode and clearly separated/documented.
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
1 participant
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.
Summary by Sourcery
Expand emulator, likelihood, and sampling capabilities for 21cmEMU v3, radio-background analyses, and advanced HERA power-spectrum inference.
New Features:
Bug Fixes:
Enhancements:
Build: