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.

Sign up for GitHub

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

Jump to bottom

Update forest.py #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update forest.py #15

wants to merge 2 commits into from

Conversation

Dhruv127
Copy link
Contributor

No description provided.

jkfitzsimons
jkfitzsimons requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@jkfitzsimons jkfitzsimons left a comment

Choose a reason for hiding this comment

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

Please check out the comments. This PR does not implement a DP RandomForrestRegressor. Hopefully the comments clarify why but if you get really stuck let me know and I can help guide you! 👍

diffprivlib/models/forest.py Outdated
self.bounds = bounds
self.random_state = random_state

def _dp_mean(self, array):
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to never be called....

diffprivlib/models/forest.py Outdated
return private_mean

def fit(self, X, y):
super().fit(X, y) # Fit the RandomForestRegressor
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are fitting with the parent RandomForrestRegressor which means no differential privacy will be applied during the training (ie the main point). Please go back an look at how they created the RandomForrestClassifier via custom trees. Their trees randomly partition the domain (not using the gini or entropy coefficient like in the vanilla RandomForrestRegressor). Then once the random trees are created you can use your _dp_mean method on the leaves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check now

@Dhruv127
Copy link
Contributor Author

@jkfitzsimons please check now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkfitzsimons jkfitzsimons jkfitzsimons requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Dhruv127 @jkfitzsimons