🩹Fix for when the backsweep is a parameter rather than constant #1422
Conversation
The optimizer would crash When setting the values in the model scheme from a constant to a parameter, the optimizer would crash with "Cannot determine Numba type of <class 'glotaran.parameter.parameter.Parameter'> " as it tried to use the parameter as a numeric value. Ultimately numba would complain as that's where the value would be first used. Specifically: `"glotaran\builtin\elements\kinetic\matrix.py", line 27`
|
|
There was a problem hiding this comment.
PR Type: Bug fix
PR Summary: The pull request addresses an issue where the optimizer would crash when a parameter was used instead of a constant numeric value for the backsweep in the model scheme. The changes include a type check and assignment to handle the Parameter object correctly.
Decision: Comment
📝 Type: 'Bug fix' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Ensure that the Parameter object is always initialized with a value attribute to prevent potential runtime errors.
- Consider using a getter method for accessing the value of the Parameter object to encapsulate the logic and provide a single point of maintenance.
- Rename the boolean variable to include a prefix that indicates its nature, such as
has_backsweep, to improve code readability. - Document the expected types of
backsweep_periodin the function's docstring or type annotations for better code clarity.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -172,6 +173,8 @@ def parameters( | |||
| scales = self.scale or [1.0] * nr_gaussians | |||
| backsweep = self.backsweep is not None | |||
| backsweep_period = self.backsweep if backsweep else 0 | |||
| if isinstance(backsweep_period, Parameter): | |||
| backsweep_period = backsweep_period.value | |||
There was a problem hiding this comment.
issue (llm): Directly accessing the value attribute of a Parameter object could be risky if there's any chance the Parameter could be uninitialized or if the value attribute could be absent. It might be safer to use a getter method or to ensure that Parameter objects always have a value attribute.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #1422 +/- ##
=======================================
Coverage 84.9% 84.9%
=======================================
Files 91 91
Lines 3746 3746
Branches 728 728
=======================================
Hits 3182 3182
Misses 450 450
Partials 114 114 ☔ View full report in Codecov by Sentry. |
| @@ -172,6 +173,8 @@ def parameters( | |||
| scales = self.scale or [1.0] * nr_gaussians | |||
| backsweep = self.backsweep is not None | |||
| backsweep_period = self.backsweep if backsweep else 0 | |||
| if isinstance(backsweep_period, Parameter): | |||
There was a problem hiding this comment.
Since Parameter implements __float__ we could simply change the assignment to
- backsweep_period = self.backsweep if backsweep else 0
+ backsweep_period = float(self.backsweep) if backsweep else 0That way we also wouldn't need the type ignore in line 185
There was a problem hiding this comment.
Tried to adopt that suggestion ...
But:
glotaran\builtin\items\activation\gaussian.py:175: error: Argument 1 to "float" has incompatible type "Parameter | float | str | None"; expected "str | Buffer | SupportsFloat | SupportsIndex" [arg-type]
glotaran\builtin\items\activation\gaussian.py:183: error: Unused "type: ignore" comment [unused-ignore]
There was a problem hiding this comment.
That said using a pydantic Model instead of dataclass for GaussianActivationParameters should also take proper care of type coercion.
There was a problem hiding this comment.
That said using a pydantic Model instead of dataclass for GaussianActivationParameters should also take proper care of type coercion.
Since it is a drop in replacement, I have no objection to change it.
There was a problem hiding this comment.
Tried to adopt that suggestion ...
But:
glotaran\builtin\items\activation\gaussian.py:175: error: Argument 1 to "float" has incompatible type "Parameter | float | str | None"; expected "str | Buffer | SupportsFloat | SupportsIndex" [arg-type]
glotaran\builtin\items\activation\gaussian.py:183: error: Unused "type: ignore" comment [unused-ignore]
This is limitation of mypy, since it cannot understand that the type is narrowed done at that point. Either ignore it or add an assert that is of float instance.
This is possible since Parameter implements __float__
This reverts commit 3490e52.
7d5e37c to
b788092
Compare
|
|
Verified the changes work |
…aran#1422) * Fix for crashing optimizer when the backsweep is a parameter rather than constant --------- Co-authored-by: s-weigand <s.weigand.phy@gmail.com>
…lotaran#1423) Just a small additional clean-up after glotaran#1422 to get rid of the backsweep boolean flag, which is a residual of the < 0.8 model syntax when we had this as user input.
The optimizer would crash
When setting the values in the model scheme from a constant to a parameter, the optimizer would crash with "Cannot determine Numba type of <class 'glotaran.parameter.parameter.Parameter'> " as it tried to use the parameter as a numeric value. Ultimately numba would complain as that's where the value would be first used.
Specifically:
"glotaran\builtin\elements\kinetic\matrix.py", line 27Note: This PR doesn't add a unit test (yet), but that should be added at the level of the optimizer since the element tests themselves work only with numeric definitions.