Skip to content

Fix: Prevent name collision between coords and data variables #7794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: Prevent name collision between coords and data variables #7794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a383a9
Fix: Remove duplicate test methods for coord/variable name collision
google-labs-jules[bot] May 21, 2025
6079615
Update pymc/model/core.py
twiecki May 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pymc/model/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,11 @@ def add_coord(
FutureWarning,
)

if name in self.named_vars:
Copy link
Preview

Copilot AI May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.

raise ValueError(
f"Name '{name}' already exists as a variable name in the model. Please choose a different name for the dimension."
)

if name in {"draw", "chain", "__sample__"}:
raise ValueError(
"Dimensions can not be named `draw`, `chain` or `__sample__`, "
Expand Down Expand Up @@ -1463,6 +1468,10 @@ def add_named_variable(self, var, dims: tuple[str | None, ...] | None = None):
"""
if var.name is None:
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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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."

)
if self.named_vars.tree_contains(var.name):
raise ValueError(f"Variable name {var.name} already exists.")

Expand Down
10 changes: 10 additions & 0 deletions tests/model/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,16 @@ def test_setattr_properly_works(self):
assert len(submodel.value_vars) == 2
assert len(model.value_vars) == 3

def test_name_conflict_variable_and_coord(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong test class to put this test in, this is for nested models tests

with pm.Model(coords={"test_name": [1, 2, 3]}) as model1:
with pytest.raises(ValueError, match="already exists as a coordinate name"):
pm.Data("test_name", [4, 5, 6])

with pm.Model() as model2:
pm.Data("another_name", [7, 8, 9])
with pytest.raises(ValueError, match="already exists as a variable name"):
model2.add_coord("another_name", [10, 11, 12])

def test_context_passes_vars_to_parent_model(self):
with pm.Model() as model:
assert pm.model.modelcontext(None) == model
Expand Down
Loading