-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fill in codegen gaps for means #293
Conversation
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.
Good! just notice something small
@@ -1 +1,9 @@ | |||
# todo: https://github.com/opendp/dp-wizard/issues/275 | |||
print( | |||
f"DP means for COLUMN_NAME " |
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.
COLUMN_NAME here may need to be dynamic?
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 is. All of these template files substitute values where there are string in ALLCAPS.
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.
... but if you're just looking at this file in isolation, how would a new developer know that? There's another PR
that would let us pull these templates closer to where they are used, and that might help.
One little comment added about intervals. The templating could also use a better explanation, but there might be bigger changes there soon-ish. Thanks for review, @ChengShi-1. |
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.
Looks good to me
Add plotting and reporting for means
I would like to keep that issue open because this doesn't yet report accuracy for DP means.
The logic is kind of different if there are or aren't groups. I've filed an issue upstream to see if some of these differences could be ironed out
group_by([])
be allowed? opendp#2313... but I'm not sure this will be feasible.
For the reviewer: