Component equality check refactor

[momchil-flex]

This caused a validator error when comparing two semiconductor media since the N_d and N_a values can be both a float and a tuple (so when one medium had N_d a float and the other - a tuple, it was erroring).

Greptile Summary

This PR adds a type check to the component equality comparison function in tidy3d/components/base.py. The change addresses a specific bug where comparing two semiconductor media objects would fail with validator errors when they had different types for the same field (e.g., one having N_d as a float and another as a tuple).

The fix introduces an early type check using type(val1) is not type(val2) that returns False immediately if the types don't match, preventing the comparison from proceeding to validation steps that would cause errors. This change integrates well with the existing equality comparison logic in the base component system, which is fundamental to how Tidy3D objects are compared throughout the codebase.

The modification follows sound equality semantics - objects with different types for the same field should be considered unequal. This is a defensive programming approach that prevents downstream validation errors while maintaining logical correctness.

Important Files Changed

Files Modified

Filename	Score	Overview
tidy3d/components/base.py	4/5	Added early type check in equality comparison to prevent validator errors when comparing objects with different field types

Confidence score: 4/5

• This PR is safe to merge with minimal risk as it addresses a specific bug with a targeted fix
• Score reflects a simple, well-reasoned change that follows good defensive programming practices and prevents error conditions
• No files require special attention as the change is localized and straightforward

Sequence Diagram

sequenceDiagram
    participant User
    participant Component1 as "Tidy3dBaseModel (Component 1)"
    participant Component2 as "Tidy3dBaseModel (Component 2)"
    participant EqualityCheck as "__eq__ method"
    participant CheckEqual as "check_equal function"

    User->>Component1: "component1 == component2"
    Component1->>EqualityCheck: "Call __eq__(other)"
    EqualityCheck->>CheckEqual: "check_equal(self.dict(), other.dict())"
    
    CheckEqual->>CheckEqual: "Compare dictionary keys"
    alt Keys are different
        CheckEqual-->>EqualityCheck: "Return False"
    else Keys are same
        loop For each key-value pair
            CheckEqual->>CheckEqual: "Get val1 = dict1[key], val2 = dict2[key]"
            CheckEqual->>CheckEqual: "val1 = get_static(val1), val2 = get_static(val2)"
            CheckEqual->>CheckEqual: "Check type(val1) is not type(val2)"
            alt Types are different
                CheckEqual-->>EqualityCheck: "Return False"
            else Types are same
                CheckEqual->>CheckEqual: "Check if either value is None (XOR)"
                alt One is None, other is not
                    CheckEqual-->>EqualityCheck: "Return False"
                else Both None or both not None
                    alt Values are tuples
                        CheckEqual->>CheckEqual: "Convert tuples to dicts and recurse"
                    else Values are dicts
                        CheckEqual->>CheckEqual: "Recurse with dictionaries"
                    else Values are numpy arrays
                        CheckEqual->>CheckEqual: "Use np.array_equal()"
                    else Other types
                        CheckEqual->>CheckEqual: "Use == comparison"
                    end
                end
            end
        end
        CheckEqual-->>EqualityCheck: "Return True if all comparisons passed"
    end
    
    EqualityCheck-->>Component1: "Return equality result"
    Component1-->>User: "Return boolean result"

Loading

[greptile-apps]

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[momchil-flex]

I guess this is too strict, it causes some existing tests to fail.

There could be other ways to approach this, ideally I shouldn't be addressing the very specific bug that caused it though (float had no len). One.. creative solution is to capture an error and return False (maybe after logging it as an error). Any other suggestions?

[yaugenst-flex]

I actually reworked the equality check on the pydantic v2 branch and it includes a similar type check:

tidy3d/tidy3d/components/base.py

Lines 1118 to 1164 in f8c1f78

	def __eq__(self, other: object) -> bool:
	"""Two models are equal when origins match and every public or extra field matches."""
	if not isinstance(other, BaseModel):
	return NotImplemented
	self_origin = (
	getattr(self, "__pydantic_generic_metadata__", {}).get("origin") or self.__class__
	)
	other_origin = (
	getattr(other, "__pydantic_generic_metadata__", {}).get("origin") or other.__class__
	)
	if self_origin is not other_origin:
	return False
	if getattr(self, "__pydantic_extra__", None) != getattr(other, "__pydantic_extra__", None):
	return False
	def _fields_equal(a, b) -> bool:
	a = get_static(a)
	b = get_static(b)
	if a is b:
	return True
	if type(a) is not type(b):
	if not (isinstance(a, (list, tuple)) and isinstance(b, (list, tuple))):
	return False
	if isinstance(a, np.ndarray):
	return np.array_equal(a, b)
	if isinstance(a, (xr.DataArray, xr.Dataset)):
	return a.equals(b)
	if isinstance(a, Mapping):
	if a.keys() != b.keys():
	return False
	return all(_fields_equal(a[k], b[k]) for k in a)
	if isinstance(a, Sequence) and not isinstance(a, (str, bytes)):
	if len(a) != len(b):
	return False
	return all(_fields_equal(x, y) for i, (x, y) in enumerate(zip(a, b)))
	if isinstance(a, float) and isinstance(b, float) and np.isnan(a) and np.isnan(b):
	return True
	return a == b
	for name in type(self).model_fields:
	if not _fields_equal(getattr(self, name), getattr(other, name)):
	return False
	return True

tests are passing there. might be good to compare these and perhaps just update the implementation here too?

[tylerflex]

I"m a little confused, so this should work

>>> 1 == (4,3)
False

what was the error exactly?

What's a bit worrisome maybe about this is whether we want to eg consider a FieldMonitor with freqs of (1,2,3) equal to one with freqs of np.ndarray([1,2,3]). I think with this change it goes from == to !=?

[momchil-flex]

I"m a little confused, so this should work

>>> 1 == (4,3)
False

what was the error exactly?

Without this change, it gets here and it enters that if statement because one of them is a tuple. But the other one is not and the error is on len(val1) as it has no len.

[github-actions]

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

• tidy3d/components/base.py (100%)

Summary

• Total: 1 line
• Missing: 0 lines
• Coverage: 100%

[momchil-flex]

So this has been kind of annoying. A naive addition like

                if type(val1) is not type(val2):
                    if not (isinstance(val1, (list, tuple)) and isinstance(val2, (list, tuple))):
                        return False

would then make some tests error on comparisons between a float and a numpy.float64 which should return True but now return False.

I used AI to port @yaugenst-flex 's pydantic v2 version. A direct port was failing on some tests because of the different handling of lits/numpy arrays. I've brought the changes to a state that works (using AI) but I don't know if we want to go for such a big change here, given that at some point we'll be getting pydantic v2 in. On the other hand, there might be something here that could improve the pydantic v2 implementation?

I guess if we want to go with this, (A)I should add tests to complete the code coverage.

[momchil-flex]

This got buried @tylerflex @yaugenst-flex

[tylerflex]

@momchil-flex the current implementation (with AI, that makes a lot of changes) actually scares me quite a bit. If it's consistent with what @yaugenst-flex is doing for pydantic v2 maybe that's fine. But perhaps we can just fix the failing case for now, add more tests, and then wait for the v2 merge to do this more deeply? Just think with the equality checking there is a lot of opportunity for subtle bugs so I'd feel a bit more conservative about this and my preference would be for one of the earlier iterations where the immediate bug is fixed. even if it's not as general as we'd like. Thoughts?

[momchil-flex]

Fix it how?

So this has been kind of annoying. A naive addition like

                if type(val1) is not type(val2):
                    if not (isinstance(val1, (list, tuple)) and isinstance(val2, (list, tuple))):
                        return False

would then make some tests error on comparisons between a float and a numpy.float64 which should return True but now return False.

Take this approach but add even more logic?

[tylerflex]

I'm talking with @yaugenst-flex in DM who's reassuring me that it's not AI changes specifically, I might have missed some context. Ultimately it comes down to @yaugenst-flex judgement I think. If there's a way to fix this in 1 line and then do these changes in Yannick's v2 PR maybe that's ideal? if not, I'm ok with what you two decide

[tylerflex]

Hm, I think I see what you mean. I think the "proper" way to handle this might be to just declare N_a and N_d to be a single type in the schema, for example just a tuple. was that considered?

[yaugenst-flex]

so a minimal fix would probably be to flip if isinstance(val1, tuple) or isinstance(val2, tuple) to if isinstance(val1, tuple) and isinstance(val2, tuple) - want to try?
the new approach is better for correctness and robustness because it handles different data types without workarounds (tuple->dict) which prevents the original validator error here as well as similar classes of issues that probably still exist.

i don't really feel strongly either way. i don't see a problem with switching to the new implementation (since it already exists on the v2 branch and will be merged anyways) but it's also not a big deal to just do a "surgical" fix here.

[momchil-flex]

I think I'd also prefer a surgical fix because your code couldn't be ported exactly (some tests would fail, probably because of pydantic version difference), so it will definitely conflict with the code here.

if isinstance(val1, tuple) and isinstance(val2, tuple) - this might just lead to some other strange error when only one of them is as the code will progress beyond this?

[tylerflex]

Another thing to try would be to just define one format that we want to perform equality checking on. For example, if it's a tuple of length 1, we just strip out the single element for equality checking?

[yaugenst-flex]

if isinstance(val1, tuple) and isinstance(val2, tuple) - this might just lead to some other strange error when only one of them is as the code will progress beyond this?

i'm relatively confident it should work, but yes the current implementation just lends itself to leading to some other strange errors 😄

[yaugenst-flex]

Another thing to try would be to just define one format that we want to perform equality checking on. For example, if it's a tuple of length 1, we just strip out the single element for equality checking?

when comparing tuples we should just use python's built-in ==, not hack our way around it...

[tylerflex]

when comparing tuples we should just use python's built-in ==, not hack our way around it...

generally I agree, except by allowing float | tuple[float, ...] and requiring them to be equal, we are basically hacking it already right? so I'm just suggesting one way of "normalizing" the data for comparison to match what we decided we want.

[yaugenst-flex]

but 1 and [1] should not be equal, or am i misunderstanding this? as i understood, this just shouldn't error. but having them compare equal does not seem like sane behavior to me?

[momchil-flex]

Right, we don't want them to be equal. We just want it not to error.

[tylerflex]

ah ok I misunderstood.

[momchil-flex]

if isinstance(val1, tuple) and isinstance(val2, tuple) - this might just lead to some other strange error when only one of them is as the code will progress beyond this?

i'm relatively confident it should work, but yes the current implementation just lends itself to leading to some other strange errors 😄

It does work for my test, let's see if all other tests also pass.