Fix: Prevent name collision between coords and data variables #7794
Conversation
Removes two unintentionally duplicated instances of the `test_name_conflict_variable_and_coord` test method from the `TestNested` class in `tests/model/test_core.py`. This ensures the test suite is clean and avoids redundant test executions.
There was a problem hiding this comment.
Pull Request Overview
This PR adds safeguards against naming conflicts between model coordinates and variables, and includes tests to verify these checks.
- Introduces
ValueErrorinModel.add_coordif the coordinate name exists among variables. - Introduces
ValueErrorinModel.add_named_variableif the variable name exists among coordinates. - Adds a test covering both conflict scenarios between data variables and coordinates.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/model/test_core.py | Added test_name_conflict_variable_and_coord to cover both conflict directions. |
| pymc/model/core.py | Implemented collision checks in add_coord and add_named_variable. |
Comments suppressed due to low confidence (2)
pymc/model/core.py:948
- Update the
add_coorddocstring to mention that coordinate names cannot collide with existing variable names and that aValueErroris raised when such a conflict occurs.
def add_coord( tests/model/test_core.py:102
- [nitpick] Consider adding a similar test for collisions involving random variable names (e.g., using
pm.Normal) against existing coordinates to ensure full coverage across all variable types.
def test_name_conflict_variable_and_coord(self): | FutureWarning, | ||
| ) | ||
| | ||
| if name in self.named_vars: |
There was a problem hiding this comment.
[nitpick] The collision check logic in add_coord and add_named_variable is duplicated. Consider extracting a shared helper method to centralize name‐conflict validation and maintain consistent error messages.
ricardoV94 left a comment
ricardoV94 left a comment There was a problem hiding this comment.
Looks good, just a small change in the error message and agree with copilot to test with a distribution as well as data
| raise ValueError("Variable is unnamed.") | ||
| if var.name in self.coords: | ||
| raise ValueError( | ||
| f"Name '{var.name}' already exists as a coordinate name in the model. Please choose a different name for the variable." |
There was a problem hiding this comment.
| f"Name '{var.name}' already exists as a coordinate name in the model. Please choose a different name for the variable." | |
| f"Name '{var.name}' already exists as a dimension name in the model. Please choose a different name for the variable." |
| assert len(submodel.value_vars) == 2 | ||
| assert len(model.value_vars) == 3 | ||
| | ||
| def test_name_conflict_variable_and_coord(self): |
There was a problem hiding this comment.
wrong test class to put this test in, this is for nested models tests
| There is a pre-existing test that is failing due to the new error message. Importantly it suggests there was already some code to handle this. Should check why it wasn't sufficient and remove because it's now probably duplicated |
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
2 participants
Fix: Prevent name collision between coords and data variables
Adds checks to prevent you from defining a coordinate with the same name as a data variable, or vice-versa.
This addresses issue #7788, where such name collisions could lead to downstream errors, particularly with
sample_posterior_predictivereturning prior predictive samples instead of posterior predictive samples.The following changes were made:
Model.add_named_variableto check if the proposed variable name already exists as a coordinate name.Model.add_coordto check if the proposed coordinate name already exists as a variable name.tests/model/test_core.pyto verify these checks.📚 Documentation preview 📚: https://pymc--7794.org.readthedocs.build/en/7794/