-
Notifications
You must be signed in to change notification settings - Fork 43
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.
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
proper equality checking between tidy3d base model #1237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit precarious in terms of expandability / edge cases, but probably best to handle correctly yeah.
tidy3d/components/base.py
Outdated
return False | ||
|
||
elif isinstance(val1, np.ndarray) or isinstance(val2, np.ndarray): | ||
if not np.allclose(np.array(val1), np.array(val2)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This errors if val1
and val2
are arrays of different shapes. There may be other edge cases too (val1
is array while val2
is something on which np.array(val2)
errors?) How about:
try:
are_equal = np.allclose(np.array(val1), np.array(val2))
except:
return False
if not are_equal:
return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, good point. I actually saw this function recommended on the internet for something related so maybe I'll do
try:
np.testing.assert_equal(val1, val2)
except AssertionError:
return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ianwilliamson, that's a much better way to do it.
It probably makes sense for me to look at |
|
||
if isinstance(val1, tuple) or isinstance(val2, tuple): | ||
val1 = dict(zip(range(len(val1)), val1)) | ||
val2 = dict(zip(range(len(val2)), val2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_logging_warning_capture
seems to fail when it tries to compare two mediums with frequency_range == None
and frequency_range != None
. We need to take into account cases when either val1
or val2
is None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks for taking a look, I'd add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding
if (val1 is None) ^ (val2 is None):
return False
makes everything pass for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if val1 is None and val2 is None, shouldn't it be True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait. this is exclusive or I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, apparently this is XOR in Python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is a bit-wise XOR operator, not the logical one. Not that it matters in this case, but the logical XOR would be (val1 is None) != (val2 is None)
a9bda7e
to
1dd8082
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lists cannot be part of our models, right? That would be the only missing case that I can think of that is not covered by the general ==
test.
tidy3d/components/base.py
Outdated
val2 = dict2[key] | ||
|
||
# if one of val1 or val2 is None (exclusive OR) | ||
if (val1 is None) ^ (val2 is None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be even better to check that the values have the same type (unless we want to return true if a list has the same items as a tuple, for example): if type(val1) != type(val2): return False
This would also eliminate the need for all or
s in the remaining if
clauses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one problem with this is that (I think? in ArrayLike) there can be situations where val1
is a list and val2
is a np.ndarray
with the same data. But if we want this situation to be != , then what you suggest makes a lot of sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this does seem to fail some tests, but might be worth looking into more deeply. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thing that comes up is comparing a tidy3dcomplex
and complex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few different approaches (such as trying to cast val1 to val2's type and vice versa) and it seems there are just a lot of edge cases to handle here, if you have a solution that works, let me know, otherwise I might leave it how it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, handling all those cases would be painful. It seems better to leave only the None
check. We wouldn't be able to foresee user-derived types anyways (a custom extension of float
for example).
|
||
if isinstance(val1, tuple) or isinstance(val2, tuple): | ||
val1 = dict(zip(range(len(val1)), val1)) | ||
val2 = dict(zip(range(len(val2)), val2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is a bit-wise XOR operator, not the logical one. Not that it matters in this case, but the logical XOR would be (val1 is None) != (val2 is None)
1dd8082
to
f5ebf46
Compare
Would be good to check:
Previously, we were checking equality between tidy3d components by comparing their
_json_string
s. However, these did not containDataArray
objects, so this equality checking was not complete.If we compare at a
self.dict() == other.dict()
we run into problems ifnp.ndarray
are present, because we don't make it clear we want to takeall()
.This PR does the equality checking via a recursive function, which handles various edge cases, including the
np.ndarray
.Once this was working, it actually exposed a few bugs in our test_IO because the loaded and saved sims were not the same as their data had changed due to some updates in CustomMedium. This was fixed by comparing
json_strings
in the tests.Note: this also fixes #1235 which was caused by the merging of two
CustomMedium
s when they shouldn't have been merged (due to this lax equality checking)There is one test failing in
test_log.py
@lucas-flexcompute could you look at it when you get time? I'm not sure why it would fail due to this PR, but this is the error message. Seems the simulation can't be validated?