Skip to content
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

[feat] Explainable state modified node comparisons #7102

Closed
wants to merge 1 commit into from

Conversation

z3z1ma
Copy link
Contributor

@z3z1ma z3z1ma commented Mar 2, 2023

resolves #7109

Description

Spike to try updating node comparisons to leverage an object with a __bool__ method that additionally carries requisite data to log detailed diffs during state based node selection.

Checklist

@z3z1ma z3z1ma requested a review from a team as a code owner March 2, 2023 03:46
@z3z1ma z3z1ma requested a review from emmyoop March 2, 2023 03:46
@cla-bot cla-bot bot added the cla:yes label Mar 2, 2023
@gshank
Copy link
Contributor

gshank commented Jun 14, 2023

Because of a number of changes to nodes.py and because we're in the middle of removing the old metric code, this PR is in a difficult to resolve state. Also the same_contents method has switched to doing "|" between the different checks, which is different behavior and might be breaking, so I'm thinking we don't want to do that at this time. The comparison improvements do look valuable, however. My suggestion would be to wait a bit until the metric switchover is a bit farther along, and then update this (removing the switch to "|" and try to get this included in 1.6.

@z3z1ma
Copy link
Contributor Author

z3z1ma commented Jun 14, 2023

Makes sense to me. We can close this out and I can follow up after the release with a new PR with fresh eyes.
cc: @gshank

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jun 25, 2023

Thanks @z3z1ma @gshank! For what it's worth - this is still a really cool idea, and I'd love to keep making progress toward it (for v1.7 if not for v1.6)

I'm going to close the PR for housekeeping purposes for now

@jtcohen6 jtcohen6 closed this Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2260] [Feature] Explainable state modified
3 participants