`ENH` add metric frame by lazarust · Pull Request #298 · skops-dev/skops

lazarust · 2023-02-12T03:34:09Z

Closes #278

lazarust · 2023-02-12T21:24:44Z

@BenjaminBossan I feel I should write a test for this, but I'm unsure where it should go. I was thinking of skops/card/tests/test_card.py inside the TestTableSection class. Thoughts? Is there somewhere else it would be better?

BenjaminBossan

I didn't do a thorough review yet, just high level.

I feel I should write a test for this, but I'm unsure where it should go. I was thinking of skops/card/tests/test_card.py inside the TestTableSection class. Thoughts? Is there somewhere else it would be better?

I think it would be best to add a completely new TestAddMetricFrame. You can take inspiration from other tests though.

BenjaminBossan · 2023-02-13T11:49:14Z

 from typing import Any, Iterator, Literal, Protocol, Sequence, Union

 import joblib
+from fairlearn.metrics import MetricFrame


We should not import fairlearn at the top level, since it would mean that it's a required dependency. It should be imported inside the corresponding method, similar to how you import pandas there.

However, you should not import them directly, but use the helper function we introduced, like here:

skops/skops/card/_model_card.py

Line 1130 in b324b8d

plt = import_or_raise("matplotlib.pyplot", "permutation importance")

BenjaminBossan · 2023-02-13T11:52:45Z

+        self, metrics: dict, y_true, y_pred, sensitive_features, pivot=True
+    ) -> Card:
+        """
+        Add a metric frame table to the model card.


About the name add_metric_frame, I'm just wondering out loud if we can find a better name. For users who don't know fairlearn, the name could be very confusing, and the docstring "Add a metric frame table to the model card" doesn't add much.

BenjaminBossan · 2023-02-14T10:22:06Z

@lazarust I see that there are some black issues. If you set up your dev environment with pre-commit hooks as described here, they should be caught before committing.

lazarust · 2023-02-16T03:19:21Z

@BenjaminBossan Thanks for the tip! Sorry, I missed that from the Contribution Guide. I'm currently working on making the tests. I'll ping you whenever it's ready again!

lazarust · 2023-02-19T17:16:37Z

@BenjaminBossan This is ready for you to take a look at!

BenjaminBossan

Thanks a lot for this. I have a couple of code comments, but before considering those, I would like to discuss a bigger design decision.

Right now, the user more or less passes the arguments for MetricFrame to the add_fairlearn_metric_frame method, which takes care of creating the MetricFrame instance. I would propose to change this so that the user has to create the MetricFrame instance themselves, then pass it to add_fairlearn_metric_frame ("inversion of control"), which does not need to construct it but just takes care of creating and adding the table.

This approach is similar to add_permutation_importances, which takes the computed permutation importances as input, instead of computing them inside the method.

Why do I think this could be better? Here are a few reasons:

With the current implementation, the user loses control over the instantiation of MetricFrame. Therefore, if they want to use something like control_features or sample_params, it's not possible. Of course, we could add more parameters to the signature of add_fairlearn_metric_frame, but it only gets bigger and bigger that way, and we have to keep it up to date when fairlearn changes.

Another advantage of having the user pass the instance is that we don't need to import fairlearn inside the method. If a user creates that object, they have already imported fairlearn.

One more advantage is that if a user has a custom MetricFrame class, they can pass that to the method, whereas right now, it's impossible to use. For testing, we could even create a mock object instead of using a real MetricFrame, and then skops would have no dependency on fairlearn at all! But I think adding a test dependency is fine here.

A disadvantage of my proposal is that users have to do a little bit of extra work by instantiating the object themselves.

Overall, I think this price is worth paying. What do you think? If we decide to make this change and you refactor the code accordingly, then many of my comments are obsolete. I think you will see which ones.

BenjaminBossan · 2023-02-20T12:41:15Z

+        y_pred,
+        sensitive_features,
+        table_name: str,
+        pivot=True,


Do we really need the pivot option? Also, is it not "transpose" more than "pivot"?

Hmmm, I guess we don't really need the pivot if we don't want to.

So if we don't transpose it the table looks like:

difference group_max group_min ratio

selection_rate 0.4 0.8 0.4 0.5

But when transposing it looks like:

selection_rate

difference 0.4

group_max 0.8

group_min 0.4

ratio 0.5

Personally, I see the transposed version as more useful so we could just make that one the version of the table that is generated. What do you think?

I'm fine changing the name of the parameter to transpose if we decide to keep it.

BenjaminBossan · 2023-02-20T12:43:25Z

+        """
+        Add a Fairlearn MetricFrame table to the model card.


Suggested change

"""

Add a Fairlearn MetricFrame table to the model card.

"""Add a Fairlearn MetricFrame table to the model card.

Could you also add a few words of description here, + a link to to fairlearn? If you want to link to the class inside fairlearn docs, you would need to add an entry to the intersphinx mappings:

skops/docs/conf.py

Line 145 in 3e1f138

intersphinx_mapping = {

BenjaminBossan · 2023-02-20T12:44:55Z

+
+        Parameters
+        ----------
+        metrics: dict


According to the fairlearn docs, metrics can also be a callable. I think the outputs of metric_frame.difference() etc. will be scalars instead of lists, which is a bit annoying. So when the table is created, we might need to add np.atleast_1d(metric_frame.difference()) or something like this to support callables.

lazarust · 2023-02-21T01:25:14Z

@BenjaminBossan Thanks for the review! I haven't looked at your code comments yet since the decision of whether to pass in the parameters or a MetricFrame affects a lot of it.

Of course, we could add more parameters to the signature of add_fairlearn_metric_frame, but it only gets bigger and bigger that way, and we have to keep it up to date when fairlearn changes.

I agree! I was thinking of that while writing the method but was unsure if that was something we wanted to do.

I do wonder if we require users to install and import fairlearn on their own if this feature just won't be known/used as much. I could be completely off base on that, but it is a thought I have. We could add an example of using this or just add it to the existing examples.

Beyond that, I don't really see too many disadvantages that would outway the advantages. What're your thoughts?

BenjaminBossan · 2023-02-21T10:10:16Z

I do wonder if we require users to install and import fairlearn on their own if this feature just won't be known/used as much.

We would do that anyway, fairlearn would not be installed by default when installing skops. And even if it were, users don't just discover and try out random packages that were installed :)

We could add an example of using this or just add it to the existing examples.

This + documenting it well are definitely the way to go for users to discover and use the feature.

Beyond that, I don't really see too many disadvantages that would outway the advantages. What're your thoughts?

Great, I think we agree then.

lazarust · 2023-02-25T22:26:08Z

@BenjaminBossan Thanks for explaining this! I think I've addressed/responded to most of your comments.

I still need to handle scalars from the MetricFrame and update the doc string.

BenjaminBossan · 2023-02-27T14:27:56Z

Great, thanks, please ping Adrin and me once it's ready for review.

lazarust · 2023-03-01T01:08:05Z

@BenjaminBossan and @adrinjalali This is ready to review again!!

adrinjalali

This looks pretty good to me, and we should probably incorporate it in our existing examples, but that could be a separate PR.

adrinjalali · 2023-03-01T08:40:07Z

+    def add_fairlearn_metric_frame(
+        self,
+        metric_frame,
+        table_name: str = "Fairlearn MetricFrame Table",
+        transpose=True,
+    ) -> Card:


should we be adding a description to our add_* methods? From the user's perspective, it seems odd that it's tricky to add a description here for sections. I think it'd make sense for all of them to allow a description as well as a title. WDYT? also cc @skops-dev/maintainers

Yes, I agree it would be good to have, some existing methods also don't have it, e.g. add_table. It's probably easier to have a separate PR where this argument is added.

@adrinjalali For clarification, should I add the description argument to this method in this PR? Or do it in another one?

we can keep it for another PR as @BenjaminBossan suggested.

One more thing: Could you please change the type annotation to return -> Self? This is a change we recently made on all the other methods as well.

adrinjalali · 2023-03-01T08:48:59Z

+        -------
+        self: Card
+            The model card with the metric frame added.
+        """


Suggested change

"""

Notes

--------

You can check `fairlearn's documentation

<https://fairlearn.org/v0.8/user_guide/assessment/index.html>`__ on how to

work with `MetricFrame`s.

"""

BenjaminBossan

Not much to add from my side. I think the transpose feature could be implemented without pandas, but it's not a big deal, most users will probably have pandas installed anyway.

adrinjalali · 2023-03-01T15:37:20Z

I think the transpose feature could be implemented without pandas, but it's not a big deal, most users will probably have pandas installed anyway.

fairlearn depends on pandas anyway.

BenjaminBossan · 2023-03-01T16:09:18Z

fairlearn depends on pandas anyway.

I didn't even think about that, then forget what I said :)

lazarust · 2023-03-02T03:17:04Z

@adrinjalali and @BenjaminBossan Thanks for reviewing this! I've addressed all the comments except for one I needed clarification on.

adrinjalali

LGTM.

BenjaminBossan

Fantastic, just this minor comment, then we can merge.

BenjaminBossan · 2023-03-02T09:19:41Z

+    def add_fairlearn_metric_frame(
+        self,
+        metric_frame,
+        table_name: str = "Fairlearn MetricFrame Table",
+        transpose=True,
+    ) -> Card:


One more thing: Could you please change the type annotation to return -> Self? This is a change we recently made on all the other methods as well.

lazarust · 2023-03-07T01:05:20Z

@BenjaminBossan I've addressed your comment! Sorry, it took me a couple of days to get to this.

BenjaminBossan

Great. Thanks a lot for your contributions, really appreciated.

We merged skops-dev#298 and skops-dev#310 shortly after each other but they contained an incompatibility that broke the fairlearn tests (the code itself was fine). This PR fixes this incompatibility. On top, I added the description argument to add_fairlearn_metric_frame, to be consistent with all the other methods, and also as a test for it. Finally, 2 small fixes: - Added type annotation to transpose argument - Changed order of arguments in docstring to match order in signature

We merged #298 and #310 shortly after each other but they contained an incompatibility that broke the fairlearn tests (the code itself was fine). This PR fixes this incompatibility. To be clear, the only change needed to fix the tests is the following: ```python - actual_table = card.select("Metric Frame Table").content.format() + actual_table = card.select("Metric Frame Table").format() ``` On top, I added the `description` argument to `add_fairlearn_metric_frame`, to be consistent with all the other methods (also changed in #310), and I also added as a test for it. Since we now have 2 tests, I moved the `metric_frame` variable to a fixture. Finally, 2 small fixes: - Added type annotation to transpose argument - Changed order of arguments in docstring to match order in signature

lazarust added 5 commits February 11, 2023 21:20

Gets add_metric_frame method started

bcc4085

Adds fairlearn to _min_dependencies

240876f

Still need to add test

9387739

Update fairlearn in _min_dependencies

09fe6d5

Sort imports

c1aa696

lazarust changed the title ~~Enh add metric frame~~ ENH add metric frame Feb 12, 2023

lazarust added 2 commits February 12, 2023 15:18

Updates changelog and adds documentation comment

0649a70

Cleans up tabs

d1b0663

lazarust marked this pull request as ready for review February 12, 2023 21:22

BenjaminBossan reviewed Feb 13, 2023

View reviewed changes

lazarust added 2 commits February 13, 2023 20:29

Removes high level import

604b79c

WIP Starting test

16d33d3

lazarust and others added 2 commits February 19, 2023 11:07

Finishes test

4c4ab22

Merge branch 'main' into ENH-add-metric-frame

321079c

BenjaminBossan requested changes Feb 20, 2023

View reviewed changes

WIP updates to pass MetricFrame directly into card

c451fcd

lazarust and others added 6 commits February 28, 2023 18:46

Fixes linke to user and adds fairlearn link in docs

9714766

WIP fix doc linking

ffcf9c9

Wip actually update docs

601e141

WIP fixes link

b55fd1e

Updates docstring

bb956b7

Merge branch 'main' into ENH-add-metric-frame

d8372ab

lazarust requested a review from BenjaminBossan March 1, 2023 01:23

adrinjalali reviewed Mar 1, 2023

View reviewed changes

BenjaminBossan reviewed Mar 1, 2023

View reviewed changes

lazarust mentioned this pull request Mar 2, 2023

ENH Adds ability to create table of contents #305

Merged

Fixes from feedback

d5477b5

adrinjalali approved these changes Mar 2, 2023

View reviewed changes

BenjaminBossan requested changes Mar 2, 2023

View reviewed changes

BenjaminBossan mentioned this pull request Mar 2, 2023

Ensure that all add_* methods on Card have a description keyword #309

Closed

lazarust and others added 2 commits March 6, 2023 18:58

Merge branch 'main' into ENH-add-metric-frame

edc5920

Switches typo annotation from Card to Self

96e6f51

lazarust requested a review from BenjaminBossan March 7, 2023 01:04

BenjaminBossan approved these changes Mar 7, 2023

View reviewed changes

BenjaminBossan merged commit 5b3a7a4 into skops-dev:main Mar 7, 2023

BenjaminBossan mentioned this pull request Mar 7, 2023

BUG: Fix bug in tests for fairlearn + 1 more test #313

Merged

	"""
	Add a Fairlearn MetricFrame table to the model card.
	"""Add a Fairlearn MetricFrame table to the model card.

-        """
+	Notes
+	--------
+	You can check `fairlearn's documentation
+	<https://fairlearn.org/v0.8/user_guide/assessment/index.html>`__ on how to
+	work with `MetricFrame`s.
+        """

Conversation

lazarust commented Feb 12, 2023

Uh oh!

lazarust commented Feb 12, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Feb 14, 2023

Uh oh!

lazarust commented Feb 16, 2023

Uh oh!

lazarust commented Feb 19, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lazarust commented Feb 21, 2023

Uh oh!

BenjaminBossan commented Feb 21, 2023

Uh oh!

lazarust commented Feb 25, 2023

Uh oh!

BenjaminBossan commented Feb 27, 2023

Uh oh!

lazarust commented Mar 1, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Mar 1, 2023

Uh oh!

BenjaminBossan commented Mar 1, 2023

Uh oh!

lazarust commented Mar 2, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lazarust commented Mar 7, 2023

Uh oh!