Tighten exception types and wrap missing-cell lookups#3380
Tighten exception types and wrap missing-cell lookups#3380
Conversation
|
Performance benchmarks:
|
| of recursion to avoid RecursionError on large radius values. | ||
| """ | ||
| if radius < 1: | ||
| raise ValueError("radius must be larger than one") |
There was a problem hiding this comment.
This is an overarching comment: where normal python errors are clearly appropriate, I prefer to use those. So here, value error is fully appropriate and I fail to see a need to replace those with custom mesa errors.
There was a problem hiding this comment.
As far as I can tell, you only changed this line. Can you carefully review the PR and if you are replacing a value or type error with a custom error, indicate why you think that is appropriate?
There was a problem hiding this comment.
I've refined the exception migration based on your feedback. I've focused the custom MesaException hierarchy on cases where it provides domain-specific context or hides internal implementation details.
For standard argument validation (type checking, value ranges), I've reverted to using built-in ValueError and TypeError as they are more than sufficient for those tasks and follow standard Python conventions.
| try: | ||
| return self._cells[key] | ||
| except KeyError as e: | ||
| raise CellMissingException(key) from e |
There was a problem hiding this comment.
Here, I am fine with wrapping the key error. It is not obvious to the user that DiscretSpace uses a dictionary internally.
Added error handling for incompatible default values.
| raise ValueError("Torus must be a boolean.") | ||
| raise TypeError("Torus must be a boolean.") | ||
| if self.capacity is not None and not isinstance(self.capacity, float | int): | ||
| raise ValueError("Capacity must be a number or None.") | ||
| raise TypeError("Capacity must be a number or None.") |
There was a problem hiding this comment.
Changed to TypeError because this is type validation, not value-range validation.
| node_positions = layout | ||
| else: | ||
| raise ValueError( | ||
| raise TypeError( |
There was a problem hiding this comment.
this is validating the type/form of layout (Mapping or Callable expected). Invalid type should be TypeError
| G.add_node(1) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| with pytest.raises(TypeError): |
There was a problem hiding this comment.
test must match production behavior after Network(..., layout=[]), changed to raise TypeError.
falloficarus22
force-pushed
the
exception-hierarchy-migration
branch
from
8ff6297 to
f8140be
Compare
falloficarus22
changed the title
discrete_space exceptions to MesaException hierarchy
|
I narrowed the scope of this PR: Keep built-in exceptions for standard argument validation ( |
|
@falloficarus22 thanks for the update. I'll try to review later today. Might it be an idea to write up something in the contributers guidlines sumarizing how to handle exceptions in Mesa? |
This is indeed important. We can add a subsection under testing and code standards in CONTRIBUTING.md. |
2 participants
Summary
This PR applies a targeted exception cleanup in
discrete_space:DiscreteSpace.__getitem__now raisesCellMissingExceptioninstead of leaking internalKeyError.TypeErrorfor wrong argument types (Grid.torus,Grid.capacity,Network.layout).ValueErrorfor invalid-but-well-typed values (e.g., out-of-bounds non-torus grid lookups).What changed
DiscreteSpace.__getitem__now wraps internalKeyErrorasCellMissingException.TypeError(instead ofValueError) where appropriate:Grid._validate_parameters:torusmust beboolcapacitymust beint | float | NoneNetwork.__init__:layoutmust beMappingorCallableGrid.find_nearest_cellto match actual raised exception (ValueErrorfor out-of-bounds non-torus positions).Tests
CellMissingExceptionon invalid coordinate access.TypeErrorfor type mismatches.TypeErrorfor invalidNetwork(layout=...).