Draft: Make noise selection a rewriting rule #83
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Some initial thoughts:
|
Contributor
Author
You're right and I did a lot of work for no good reason :D I'll close this PR and use the simpler approach instead: #94 (not ready yet, I need to fix tests).
This is probably worth more discussion — I think it might be good to do that, but this would move this logic from Core to Analytics, which I think is less consensual than changing the logic in Analytics. So I will start with the latter and then we'll see whether it makes sense to put even more in rewrite rules. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This seemed like a simple first step to implementing #23, and start moving logic from the measurement visitor to a series of rewrite rules. This PR does this for noise selection, annotating the
QueryExprs corresponding to numerical aggregations with acore_mechanismfield, and using this field during compilation.It also creates the basic plumbing for rule rewriting, similarly to what we do for KeySets. As part of that plumbing, it also adds a new field to all
QueryExprcontaining compilation info, and a rule to fill this field, so the rewrite rules can access the information they need.That last change requires us to make an additional change to
QueryExprclasses, and use thekw_only=Trueargument, which requires all callers to createQueryExprusing keywords for all arguments. Otherwise, dataclasses cannot have a field with a default value in a base class and fields with no default values in a subclass — the alternative is to have default values everywhere, which seems error-prone, or switch toattrs, which seems more complicated. This change makes a bunch of tests a little more verbose/explicit (and is responsible for most of the line changes in this PR).As a smaller drive-by fix, it changes
ReplaceInfinityto use a custom__post_init__instead of a custom__init__, so everyQueryExprworks the same way. The core logic for noise selection has also been made slightly simpler (without any behavior change).