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

Remove ExponentiatedGradientResult and flatten contents within ExponentiatedGradient object #338

Merged
merged 4 commits into from
Mar 23, 2020
Merged

Remove ExponentiatedGradientResult and flatten contents within ExponentiatedGradient object #338

merged 4 commits into from
Mar 23, 2020

Conversation

romanlutz
Copy link
Member

This is another step related to fairlearn/fairlearn-proposals#4

Removing the result object is something we've wanted to do for a while. The result object is a remnant from fairlearn<v0.3. It existed since ExponentiatedGradient was not a class, but simply a function. When we transitioned to actual classes we simply added the result object as a member with the goal to revisit this decision at a later point which we've now reached.

Roman Lutz added 2 commits March 18, 2020 21:28
remove expgrad result object
f623089 
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
fix syntax error
b185541 
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
@romanlutz romanlutz added the enhancement New feature or request label Mar 20, 2020
@romanlutz romanlutz added this to the short-term API improvements milestone Mar 20, 2020
@romanlutz romanlutz self-assigned this Mar 20, 2020
@romanlutz romanlutz mentioned this pull request Mar 20, 2020
Closed
riedgar-ms
riedgar-ms reviewed Mar 20, 2020
View reviewed changes
fairlearn/reductions/_exponentiated_gradient/exponentiated_gradient.py Outdated
self._best_classifier = lambda X: _mean_pred(X, hs, self._weights)
self._best_gap = gaps[self._best_t]
self._classifiers = lagrangian.classifiers
self._n_oracle_calls = lagrangian.n_oracle_calls
Copy link
Member

Choose a reason for hiding this comment

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

If these are supposed to be public, shouldn't they have a trailing underscore and not a leading one, to match sklearn conventions?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not 100% clear which ones are meant to be public right now. There's another work item to match naming conventions. I stuck to what's been done so far. All of the former result object contents could be public in my opinion, but maybe @MiroDudik has an opinion either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought about this a little more. Since most of these are subject to be renamed (see my proposal for an idea about what I mean fairlearn/fairlearn-proposals#4) I think it's wise to keep them private until that's happened. We don't want people to think that those attributes are there to stay (if we do a release), just to rename them in the immediately following release.
I am working through all the refactoring items suggested by @MiroDudik so I will get to #256 as well eventually. This directly targets such things, just in case I miss out on it as part of the reductions refactoring. Does that make sense?

riedgar-ms
riedgar-ms requested changes Mar 20, 2020
View reviewed changes
Copy link
Member

@riedgar-ms riedgar-ms left a comment

Choose a reason for hiding this comment

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

If the new fields are supposed to be public, shouldn't they have trailing underscores, not leading ones (as in sklearn)?

remove _format_results since it's obsolete
b0ce02e 
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
remove duplicate members
f155c45 
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
riedgar-ms
riedgar-ms approved these changes Mar 21, 2020
View reviewed changes
Copy link
Member

@riedgar-ms riedgar-ms left a comment

Choose a reason for hiding this comment

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

OK on the naming for now

MiroDudik
MiroDudik reviewed Mar 23, 2020
View reviewed changes
fairlearn/reductions/_exponentiated_gradient/exponentiated_gradient.py
self._last_t = None
self._best_t = None
self._n_oracle_calls = 0
self._oracle_calls_execution_time = None

Copy link
Member

Choose a reason for hiding this comment

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

This init is not sklearn-compliant,. Are we leaving that for a future PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't strike me as a violation, since there's no logic. But @adrinjalali will have to change a few things with reductions methods anyway, so if this is not compliant we can fix that later (since we aren't compliant yet).

MiroDudik
MiroDudik approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@MiroDudik MiroDudik left a comment

Choose a reason for hiding this comment

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

Looks good! Let's leave for future PR: renaming of the fields and making __init__ sklearn compliant.

@romanlutz romanlutz merged commit a5ce5cc into master Mar 23, 2020
@romanlutz romanlutz deleted the rolutz/reductions_result_objects_1 branch March 23, 2020 18:36
romanlutz pushed a commit that referenced this pull request Mar 31, 2020
Remove ExponentiatedGradientResult and flatten contents within Expone…
3a83f57 
…ntiatedGradient object (#338)

* remove expgrad result object

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* fix syntax error

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove _format_results since it's obsolete

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove duplicate members

Signed-off-by: Roman Lutz <rolutz@microsoft.com>
romanlutz pushed a commit that referenced this pull request Mar 31, 2020
Remove ExponentiatedGradientResult and flatten contents within Expone…
4e9f6d9 
…ntiatedGradient object (#338)

* remove expgrad result object

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* fix syntax error

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove _format_results since it's obsolete

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove duplicate members

Signed-off-by: Roman Lutz <rolutz@microsoft.com>
MiroDudik added a commit that referenced this pull request Apr 22, 2020
@mesameki @MiroDudik
@rihorn2 @riedgar-ms
Update communication guidelines in the repository (#329)
367b9e2 
* Update README.md

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Updated readme

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Update README.md

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Update README.md

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* added visualization guide and replaced fairlearn with Fairlearn

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* partial pass through README

Signed-off-by: Miro Dudik <mdudik@gmail.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* finish the pass

Signed-off-by: Miro Dudik <mdudik@gmail.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* tweaks

Signed-off-by: Miro Dudik <mdudik@gmail.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add comma

Signed-off-by: Miro Dudik <mdudik@gmail.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* removed remaining protected attribute instances

Signed-off-by: Miro Dudik <mdudik@gmail.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* COMMUNICATION.md

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* space moved

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* move sections into README & CONTRIBUTING

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* change gender to binarygender

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Change reductions inheritance to use sklearn BaseEstimator and mixins (#327)

* remove Reduction class

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* cleanup remaining imports

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add changes.md (including the description of an earlier change)

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* make ExponentiatedGradient and GridSearch implement MetaEstimatorMixin instead of ClassifierMixin and RegressorMixin

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* adjust CHANGES.md to the new interface

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Remove ExponentiatedGradientResult and flatten contents within ExponentiatedGradient object (#338)

* remove expgrad result object

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* fix syntax error

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove _format_results since it's obsolete

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove duplicate members

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Store lambda vectors on the ExponentiatedGradient object (#344)

* retain lambda vectors

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* update names for members

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* change capitalization of string (#334)

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Rework creation of group metric sets (#333)

Make `_create_group_metric_set()` private. Also, change the arguments and innards to require dictionaries for the predictions and sensitive features. This will require users to name each, which will nudge them towards not confusing themselves.

This is a breaking change, but the routine is unlikely to have users right now.

Signed-off-by: Richard Edgar <riedgar@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Add tests for _GridGenerator and documentation for the basis (#340)

* remove expgrad result object

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* fix syntax error

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add test for grid generator

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove duplicate members

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add comment about basis

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* generalize tests to parametrize them

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* flake8

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add more test cases, remove _GridGenerator from __init__.py

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* slightly adjust comment

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* address feedback on comments

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* adjust comment

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add smoke tests for postprocessing (#350)

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Fix for _create_group_metric_set() (#355)

It appears that `LabelEncoder()` returns `numpy.int64` values. However, downstream users will need regular Python integers for JSON serialisation.

Signed-off-by: Richard Edgar <riedgar@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add Gitter and badge

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* update CONTRIBUTING with Gitter

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* Update README.md

Signed-off-by: Mers Sameki <mesameki@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* .

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* undo bad merge result

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* update based on feedback by moving links into single lines

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* remove duplicate question bullet point, and add description for fork

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* add StackOverflow badge

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

* adjust non-security issues paragraph, enhance config.yml for issue templates to link to StackOverflow and Gitter directly

Signed-off-by: Roman Lutz <rolutz@microsoft.com>

Co-authored-by: Mehrnoosh Sameki <47459440+mesameki@users.noreply.github.com>
Co-authored-by: Mers Sameki <mesameki@microsoft.com>
Co-authored-by: Miro Dudik <mdudik@gmail.com>
Co-authored-by: Brandon Horn <rihorn@microsoft.com>
Co-authored-by: Richard Edgar <riedgar@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riedgar-ms riedgar-ms riedgar-ms approved these changes

@MiroDudik MiroDudik MiroDudik approved these changes

Assignees

@romanlutz romanlutz

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romanlutz @MiroDudik @riedgar-ms