Add Parameters.update() to avoid needless recomputation#294
Merged
benluijten merged 13 commits intomainfrom Mar 23, 2026
Merged
Add Parameters.update() to avoid needless recomputation#294benluijten merged 13 commits intomainfrom
Parameters.update() to avoid needless recomputation#294benluijten merged 13 commits intomainfrom
Conversation
…re equal to current parameters, and skip update if this is the case. Included a 'force' flag to force update all parameters.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Parameters.update(force=False, **kwargs) to bulk-apply changes for keys in VALID_PARAMS; by default skips assignments when new and old values are equal (ndarray equality via np.array_equal); force=True forces assignment; unknown keys are ignored; assignments go through setattr for validation and cache invalidation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
zea/internal/parameters.py (1)
366-367: Consider adding diagnostics for ignored keys inupdate().Silently ignoring unknown keys makes typo-driven no-ops hard to spot. A debug log (similar to
standardize_paramson Line 594) would improve debuggability without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zea/internal/parameters.py` around lines 366 - 367, The update() method currently silently skips keys not in VALID_PARAMS; add a debug-level diagnostic there to log each ignored key (and its source/value) so typos are visible without altering behavior. Follow the pattern used in standardize_params (same module) for log formatting and logger usage; locate the check in update() that references self.VALID_PARAMS and insert a debug log call before the continue, including the key name and the provided value/source context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zea/internal/parameters.py`:
- Around line 370-373: The update logic conflates “not set” with None because it
uses self._params.get(key); change it to detect presence explicitly (e.g., use a
unique sentinel or check `key in self._params`) so you can distinguish an unset
param from one set to None; then adjust the conditional around old_val/new_val
(references: self._params.get, old_val, new_val, update, setattr) to skip only
when both old and new are present and None or when both are absent, but allow
update(x=None) to run when the key was previously absent so dependency
resolution/state remains correct.
- Around line 374-381: The fast-path equality check in the parameters update
uses "if isinstance(old_val, np.ndarray) or isinstance(new_val, np.ndarray):"
which lets np.array_equal run for array-like inputs and thereby bypass
_validate_parameter; change the condition to require both be np.ndarray (use
AND) so np.array_equal is only used when both old_val and new_val are actual
numpy arrays; this ensures list/tuple new_val values will fall through to the
normal equality/assignment path (the elif old_val == new_val branch and
subsequent setattr) and therefore trigger the existing _validate_parameter
checks for Scan.VALID_PARAMS (e.g., probe_geometry, polar_angles,
azimuth_angles, etc.).
---
Nitpick comments:
In `@zea/internal/parameters.py`:
- Around line 366-367: The update() method currently silently skips keys not in
VALID_PARAMS; add a debug-level diagnostic there to log each ignored key (and
its source/value) so typos are visible without altering behavior. Follow the
pattern used in standardize_params (same module) for log formatting and logger
usage; locate the check in update() that references self.VALID_PARAMS and insert
a debug log call before the continue, including the key name and the provided
value/source context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6be69d67-f05b-4e8e-a770-48e56892d1fa
📒 Files selected for processing (1)
zea/internal/parameters.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_scan.py (1)
357-384: Timing test is informational only.The timing comparison at lines 366-384 uses
print()for output without any assertions. This is acceptable for demonstrating the optimization benefit, but be aware that:
- The test will pass regardless of timing results
- The loop variable
iis unused (could be_for clarity)If the goal is purely informational, this is fine. If you want to guard against performance regressions, consider adding a soft assertion (e.g., that non-forced updates are measurably faster), though such assertions can be flaky in CI environments.
♻️ Optional: use underscore for unused loop variable
t0 = time.perf_counter() - for i in range(n): + for _ in range(n): # update with same value repeatedly; should be skipped by checks scan.update(n_tx=scan.n_tx) t1 = time.perf_counter() print(f"{n} updates with checks took {t1 - t0:.6f} seconds") t0 = time.perf_counter() - for i in range(n): + for _ in range(n): # force update each time, bypassing the equality checks scan.update(force=True, n_tx=scan.n_tx) t1 = time.perf_counter() print(f"{n} forced updates took {t1 - t0:.6f} seconds")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_scan.py` around lines 357 - 384, The timing loop in test_update_ignores_unknown_keys_and_timings is purely informational and uses an unused loop variable `i`; update the test to use `_` for the loop variable (e.g., "for _ in range(n):") in both timing loops to signal it's unused and optionally replace the print statements with logging or remove them if you prefer no output, while leaving the Scan.update calls (including forced updates via Scan.update(force=True, n_tx=...)) and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_scan.py`:
- Around line 357-384: The timing loop in
test_update_ignores_unknown_keys_and_timings is purely informational and uses an
unused loop variable `i`; update the test to use `_` for the loop variable
(e.g., "for _ in range(n):") in both timing loops to signal it's unused and
optionally replace the print statements with logging or remove them if you
prefer no output, while leaving the Scan.update calls (including forced updates
via Scan.update(force=True, n_tx=...)) and behavior unchanged.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
zea/internal/parameters.py (1)
375-382:⚠️ Potential issue | 🟠 MajorRequire both operands to be
np.ndarrayin the fast-path check.Line 375 currently uses
or, which allows array-like inputs (e.g., lists) to compare equal and skipsetattr, bypassing parameter type validation.Suggested fix
- if isinstance(old_val, np.ndarray) or isinstance(new_val, np.ndarray): + if isinstance(old_val, np.ndarray) and isinstance(new_val, np.ndarray): try: if np.array_equal(old_val, new_val): continueDoes numpy.array_equal accept array-like inputs (such as Python lists), and can np.array_equal(np.array([1, 2]), [1, 2]) return True?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zea/internal/parameters.py` around lines 375 - 382, The fast-path uses "if isinstance(old_val, np.ndarray) or isinstance(new_val, np.ndarray):" which lets array-like values (e.g., lists) cause np.array_equal to be used and skip setattr; change that condition to require both operands to be numpy arrays (use "and") so that np.array_equal is only called when both old_val and new_val are np.ndarray; keep the existing try/except and the fallback equality branch (the "elif old_val == new_val: continue") so non-ndarray comparisons still use normal equality and validation via setattr occurs when types differ; update the check around np.array_equal(old_val, new_val) accordingly.
🧹 Nitpick comments (1)
tests/test_parameters.py (1)
372-423: Add one regression test to lock type-validation behavior for ndarray params.Consider asserting that
update(arr=[...])raisesTypeErrorwhenarris typed asnp.ndarray, so array-like equality never masks invalid input types.Suggested test addition
+def test_update_ndarray_array_like_input_still_validates_type(): + """List input for ndarray-typed params should not bypass type validation.""" + params = DummyArrayParameters(arr=np.array([1.0, 2.0, 3.0])) + with pytest.raises(TypeError, match="expected type ndarray"): + params.update(arr=[1.0, 2.0, 3.0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_parameters.py` around lines 372 - 423, Add a regression test ensuring type validation for ndarray-typed params: create a DummyArrayParameters (or use existing DummyArrayParameters) whose arr is annotated as np.ndarray, then assert that calling params.update(arr=[1.0, 2.0, 3.0]) raises TypeError; include this alongside the other update tests (near test_update_ndarray_equality_skips_recompute) to lock behavior that array-like inputs are rejected and not silently accepted by params.update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@zea/internal/parameters.py`:
- Around line 375-382: The fast-path uses "if isinstance(old_val, np.ndarray) or
isinstance(new_val, np.ndarray):" which lets array-like values (e.g., lists)
cause np.array_equal to be used and skip setattr; change that condition to
require both operands to be numpy arrays (use "and") so that np.array_equal is
only called when both old_val and new_val are np.ndarray; keep the existing
try/except and the fallback equality branch (the "elif old_val == new_val:
continue") so non-ndarray comparisons still use normal equality and validation
via setattr occurs when types differ; update the check around
np.array_equal(old_val, new_val) accordingly.
---
Nitpick comments:
In `@tests/test_parameters.py`:
- Around line 372-423: Add a regression test ensuring type validation for
ndarray-typed params: create a DummyArrayParameters (or use existing
DummyArrayParameters) whose arr is annotated as np.ndarray, then assert that
calling params.update(arr=[1.0, 2.0, 3.0]) raises TypeError; include this
alongside the other update tests (near
test_update_ndarray_equality_skips_recompute) to lock behavior that array-like
inputs are rejected and not silently accepted by params.update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d49cc57-5fdb-4e96-b2ca-8782903b01c5
📒 Files selected for processing (3)
tests/test_parameters.pytests/test_scan.pyzea/internal/parameters.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zea/internal/parameters.py`:
- Around line 374-382: The equality check in update can raise or return
non-scalar results for mixed/custom types (old_val == new_val), aborting the
path before setattr validation; modify the branch handling non-ndarray values in
the update method to safely evaluate equality: wrap the old_val == new_val in a
try/except catching TypeError and ValueError, then inspect the result—if it's a
boolean (or numpy.bool_) and True, continue; if it's a numpy array-like result,
use np.all(...) (or np.array_equal semantics) to decide to continue; otherwise
fall through to setattr so validation still runs. Reference: the update logic
using old_exists, old_val, new_val and numpy checks for np.ndarray.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c298d278-7824-49de-bcea-2ec15900f845
📒 Files selected for processing (2)
tests/test_parameters.pyzea/internal/parameters.py
Collaborator
Author
|
Added a "defensive" fix to cache_with_dependencies to more gracefully handle edge cases. |
wesselvannierop
approved these changes
Mar 23, 2026
benluijten
added
the
efficiency
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
2 participants
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.
TLDR
Added a new
Parameters.update()helper (inherited byScan) so callers can update multiple parameter fields at once without forcing recomputation when values haven’t actually changed.Changes
Parameters.update(**kwargs, force=False)force=False) compares new vs current values and only assigns when values differ.np.array_equalfor array comparisons and==for scalars.force=Trueforces assignment for every key (same effect as callingsetattrrepeatedly --> old behavior).Scan(and otherParameterssubclasses) inherit this behavior.Motivation
In processing loops we often recreate a
Scanor reassign many parameters every iteration. Recreating objects or assigning identical values invalidates cached computed properties and triggers expensive recomputation. This change reduces unnecessary work by only assigning when values actually change.Compatibility
setattrstill works exactly as before.force=True.Summary by CodeRabbit
New Features
Behavior
Tests