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

Improve pprinting of stateful examples #4266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tybug
Copy link
Member

@tybug tybug commented Feb 10, 2025

singleton_printers does unconditional id-based replacement, which is not always correct (see tests). test_unrelated_rule_does_not_use_var_reference_repr fails on master with:

state = UnrelatedCall()
a_0 = state.add_a(a=0)
state.unrelated(value=a_0) # should be value=0

I think the main use case for the singleton printers is addressed with VarReference, and any remaining effects are imo net-negative, though they can sometimes be more readable even if not strictly correct.

Comment on lines +380 to +381
values_0 = state.f(value=[])
state.f(value=[[]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we actually do want

Suggested change
values_0 = state.f(value=[])
state.f(value=[[]])
values_0 = state.f(value=[])
state.f(value=[values_0])

because e.g. lists are mutable and so it's valuable to show the variable name which communicates the identity of the value.

I realize that this is Quite A Can Of Worms, what with not wanting values_0 = state.f(value=values_0); your motivating case in the changelog is spot-on but here and in the changed test I think the change is probably for the worse :/

Copy link
Member Author

@tybug tybug Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that this case is worse than it used to be, but it's also not a correct transformation to make in general; see SourceSameAsTargetUnclearOrigin, where we would get state.f(value=[values_0]) even if the inner empty list came from st.just([]). And one could imagine more complicated cases of building up the inner list via some alternative st.composite where this would be more misleading.

I think we have to decide which of the two evils is worse...I believe the only case worsened by this is strategies involving bundles, which isn't great but is less common than rules involving bundles. The latter case collides relatively frequently thanks to shrinking normalizing to e.g. lots of zeros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we did self.__printer.singleton_pprinters.setdefault(id(result), printer), but only for objects which are not actually singletons? i.e. excluding None, True, False, small integers; we're still going to miss some cases but it seems closer to a strict improvement on the status quo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants