-
-
Notifications
You must be signed in to change notification settings - Fork 133
feat(web): add isSameNode for duplicate quotient-node detection 🚂 #15478
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
base: feat/web/spur-edge-key
Are you sure you want to change the base?
feat(web): add isSameNode for duplicate quotient-node detection 🚂 #15478
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
403721b to
fbe4bec
Compare
a295e51 to
bb2c119
Compare
1d44f26 to
6306f5e
Compare
bb2c119 to
e69bb62
Compare
0c4a793 to
92546fa
Compare
e69bb62 to
71df397
Compare
92546fa to
240b1f6
Compare
71df397 to
1f4897f
Compare
240b1f6 to
76b3479
Compare
1f4897f to
56d7a8f
Compare
76b3479 to
2127072
Compare
56d7a8f to
d4f3a18
Compare
Build-bot: skip build:web Test-bot: test
d4f3a18 to
1755556
Compare
| if(this.model != space.model) { | ||
| return false; | ||
| } | ||
| return 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.
Do these not have parents to check for match?
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.
It's the 'root' node - there is no parent here.
darcywong00
left a comment
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.
lgtm
ermshiperete
left a comment
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.
LGTM
| } | ||
|
|
||
| isSameNode(space: SearchQuotientNode): boolean { | ||
| // Easiest cases: when the instances or their ' `spaceId` matches, we have |
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.
| // Easiest cases: when the instances or their ' `spaceId` matches, we have | |
| // Easiest cases: when the instances or their `spaceId` matches, we have |
| // We need to check if the parents match. Done naively in the manner below, this is O(N^2). | ||
| // Granted, we shouldn't have _that_ many incoming paths. | ||
| if(this.parents.find((path) => !space.parents.find((path2) => path.isSameNode(path2)))) { | ||
| 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.
Do we have to check that this and space have the same number of parents?
| // We need to check if the parents match. Done naively in the manner below, this is O(N^2). | |
| // Granted, we shouldn't have _that_ many incoming paths. | |
| if(this.parents.find((path) => !space.parents.find((path2) => path.isSameNode(path2)))) { | |
| return false; | |
| } | |
| if(this.parents.length != space.parents.length) { | |
| return false; | |
| } | |
| // We need to check if the parents match. Done naively in the manner below, this is O(N^2). | |
| // Granted, we shouldn't have _that_ many incoming paths. | |
| if(this.parents.find((path) => !space.parents.find((path2) => path.isSameNode(path2)))) { | |
| return false; | |
| } |
Continuing from #15508, this defines a method useful for determining if one SearchQuotientNode is a 100% match and duplicate for another node. While
.isSameNodeis only leveraged in unit tests within this PR, it sees use in runtime code starting with #15031.Build-bot: skip build:web
Test-bot: skip