Conversation
maxhutch
left a comment
maxhutch
left a comment
There was a problem hiding this comment.
Overall, this looks like a reasonable approach. It'd be nice to get a bit more test coverage of the material history crawling logic; the unit tests added all look very simple. A few questions inline.
| return self._output_material | ||
|
|
||
| # To avoid infinite recursion, fast return on revisit | ||
| global eq_seen |
There was a problem hiding this comment.
Is this just a global because you can't change the signature of __eq__? Why put the global in the class instead of outside of it?
There was a problem hiding this comment.
Yes; you can't change the sig of eq and flow of control moves between several modules / is re-entrant. I wish there was a way to get a tighter scope, but a closure wouldn't get ahead of it. It's possible there's a better thread/exception safe approach, but I was also trying to maintain velocity.
Not opposed to moving the declaration to the top of file, but wrote it this way to be close to where it is used in the file.
There was a problem hiding this comment.
Better approach implemented, using the object itself as the state vector.
| else: | ||
| result = False | ||
|
|
||
| eq_seen.remove((self, other)) |
There was a problem hiding this comment.
This might be missed if an exception is raised between the equ_seen.add and this. I suggest using a try/catch/finally to make sure its removed.
There was a problem hiding this comment.
I'd initially skipped the try-catch because of performance concerns, but it seems like this would be prudent.
| # Note the hash function checks if objects are identical, as opposed to the equals method, | ||
| # which checks if fields are equal. This is because BaseEntities are fundamentally | ||
| # mutable objects. | ||
| def __hash__(self): |
There was a problem hiding this comment.
What's the purpose of this definition? Doesn't it automatically inherit from its parent?
There was a problem hiding this comment.
It actually does not; you need to explicitly define a __hash__ function every time you define an __eq__. I went into this expecting ^^ would have been correct.
There was a problem hiding this comment.
There was a problem hiding this comment.
That might be worth mentioning in the inline comments
|
|
||
| def __eq__(self, other): | ||
| if (self, other) in eq_seen: # Cycle encountered | ||
| return True # This will functionally be & with the correct result of == |
There was a problem hiding this comment.
Does this get lit up in the tests?
There was a problem hiding this comment.
Yes. When the Process Spec iterates over its Ingredient Specs, each ingredient.process bounces through this check.
maxhutch
left a comment
maxhutch
left a comment
There was a problem hiding this comment.
LGTM. It might benefit from a bit more developer docs in the comments.
| result = all(ing in other.ingredients for ing in self.ingredients) | ||
| elif (not self.ingredients and self.uids) \ | ||
| or (not other.ingredients and other.uids): | ||
| result = True # One can be empty if you flattened |
There was a problem hiding this comment.
So they are equal if they have matching ingredients OR one of them doesn't have any ingredients defined?
There was a problem hiding this comment.
Mostly correct. If they have matching ingredients OR (one of them has no ingredients AND there was a UID dictionary to validate).
Intended to compensate for the fact that ingredients is set implicitly by a relationship that wouldn't be initialized if you just compared the result of dataset.process_runs.get() in citrine-python.
There was a problem hiding this comment.
Can we make and self.uids and and other.uids a little more explicit? The way that python automatically casts structs to booleans is really easy to mess up / easy to be confused by when reading. Logically, this makes sense, though.
There was a problem hiding this comment.
Those aren't compared here - they are tested in DictSerializable.__eq__. I'm intending to make sure that any uid exists. I would be open to changing that line to (len() == 0 / len() != 0) if that reads better.
There was a problem hiding this comment.
Yeah, that's what I meant.
2 participants
As per PLA-8254, this implements GEMD object comparison, crawling over an entire material history and respecting the relationship between LinkByUIDs and the Base Entities they reference.