Metric refactor #69

jewelltaylor · 2023-11-04T02:40:15Z

PR Type

[Feature | Fix | Documentation | Other() ]

Short Description

Refactor client metrics so that they maintain state, eliminating the need for MetricMeter's which tend to just make things more complicated. Also add the option to return features from models through predict method to be used in loss computation. Then apply this to the MOON client to compute contrastive loss. Added some documentation with proper formatting to Metric related code and a few client methods that I altered slightly in this PR. In a follow up PR, I will more extensively apply the proper formatting to client code and more broadly.

Note: This should only be reviewed once the Create Fixed Requirements File for FLamby, Update Dynamic Weight Exchanger and FedOpt Example PR is merged. I just wanted to base my PR on that branch to adapt David's CustomMetricMeter (and a few other relevant parts) to the simplified Metric tracking.

Tests Added

Adapted Metric related tests that were affected in the refactor
Add a test for the updated MetricMeter

…now baked into Metric class. User only needs to define __call__ method to calculate metric given inputs and targets. Also changed the return type of predict so that we can pass predictions and features in two seperate dictionairies. Small changes to examples to integrate in aforementioned changes

… PR.

…ctionairies for predictions and features, bringing it in line with other methods

…tions and features. Also added some comments throughout

jewelltaylor · 2023-11-17T21:19:26Z

Now that PR #68 is approved and will presumably be merged into main with minimal changes, I think this PR is ready to review. I had to make some additional changes to update the parts of the code affected by Sana's recent PR #66. I would appreciate feedback that anyone has!

emersodb · 2023-11-20T15:09:26Z

examples/fedopt_example/metrics.py

+        self.n_classes: int
+        self.outcome_dict: Dict[str, Outcome]
+
+    def _setup(self, label_encoder: LabelEncoder) -> None:


Typically the _ prefix is reserved for protected or private methods. That is, methods that are exclusively called within the class itself, rather than externally. All that is to say, I would recommend dropping the _ based on the way this is being used 🙂

Yeah, good call! I initially was aiming for it to be internal but realized we have to call it externally so I forgot to change it back

emersodb · 2023-11-20T15:10:53Z

examples/fedopt_example/metrics.py


        Args:
-            label_encoder (LabelEncoder): This class is used to determine the mapping of integers to label names for


Maybe transfer this comment about label_encoder to the setup method below?

emersodb · 2023-11-20T15:16:00Z

examples/fedopt_example/client.py

@@ -99,5 +86,5 @@ def predict(self, input: torch.Tensor) -> Dict[str, torch.Tensor]:
    # Load model and data
    DEVICE = torch.device("cuda:0" if torch.cuda.is_available() else "cpu")
    data_path = Path(args.dataset_path)
-    client = NewsClassifierClient(data_path, DEVICE)
+    client = NewsClassifierClient(data_path, [CompoundMetric("")], DEVICE)


Should we give this metric a name?

emersodb · 2023-11-20T15:24:34Z

fl4health/clients/basic_client.py

-            "prediction": MetricMeter.get_meter_by_type(self.metrics, metric_meter_type, "val_meter")
-        }
-        self.val_metric_meter_mngr = MetricMeterManager(val_key_to_meter_map)
+        self.train_metric_meter_mngr = MetricManager(metrics=self.metrics, metric_mngr_name="train")


Since these are now "MetricManager" objects, should we change the names from train_metric_meter_mngr to train_metric_manager and val_metric_meter_mngr to val_metric_manager throughout?

emersodb · 2023-11-20T15:27:28Z

fl4health/utils/metrics.py

-    Class to manage one or metric meters.
-    """
+        Args:
+            preds (Dict[str, torch.Tensor]): A dictionairy of


This comment is incomplete. Also, I think there are a few places throughout the code where "dictionary" is spelled incorrectly in this way 😂

emersodb · 2023-11-20T15:28:34Z

fl4health/clients/basic_client.py

+        Returns:
+            Tuple[Dict[str, torch.Tensor], Dict[str, torch.Tensor]]: A tuple in which the first element
+            contains predictions indexed by name and the second element contains intermediate activations
+            index by name. BY passing features, we can compute losses such as the model contrasting loss in MOON.


I think you mean to capitalize BY, but just want to check

emersodb · 2023-11-20T15:29:33Z

fl4health/clients/basic_client.py

+
+        Returns:
+            Tuple[Dict[str, torch.Tensor], Dict[str, torch.Tensor]]: A tuple in which the first element
+            contains predictions indexed by name and the second element contains intermediate activations


It's probably worth mentioning that anything stored in predictions will be used to compute metrics, that way people don't just store a bunch of stuff in there accidentally?

I see you added a comment in the compute loss function about anything stored in preds being used to compute metrics, but I think we should also put that in the comment for the predict function since that's where people will define what's in the predictions dictionary. So they "know" this before potentially stuffing extra things into that dictionary.

emersodb · 2023-11-20T15:36:32Z

fl4health/clients/basic_client.py

-        In the default case, the dict has a single item with key prediction.
-        In more complicated approaches such as APFL, the dict has as many items as prediction types
-        User can override for more complex logic.
+        Computes the prediction(s) (and potentially features) of the model(s) given the input.


Super minor, but I think you can drop the parentheses here and just write

Computes the prediction(s), and optionally features, of the model(s) given the input.

emersodb · 2023-11-20T15:40:43Z

fl4health/clients/evaluate_client.py

@@ -42,9 +41,9 @@ def __init__(
        self.data_loader: DataLoader
        self.criterion: _Loss
        self.global_loss_meter = LossMeter.get_meter_by_type(loss_meter_type)
-        self.global_metric_meter = MetricMeter.get_meter_by_type(self.metrics, metric_meter_type, "global_eval_meter")
+        self.global_metric_meter = MetricManager(self.metrics, "global_eval_meter")


Maybe change the names of these properties to be something like global_metric_manager and local_metric_manager?

emersodb · 2023-11-20T15:42:38Z

fl4health/clients/fed_prox_client.py

+        self, preds: Dict[str, torch.Tensor], features: Dict[str, torch.Tensor], target: torch.Tensor
+    ) -> Losses:
+        """
+        Computes loss given predictions of the model and ground truth data.


Maybe add a description that we're also adding in the proximal loss, comparing the l2 norm between the initial and final weights of local training

emersodb · 2023-11-20T15:45:10Z

fl4health/clients/fenda_client.py

+
+        Returns:
+            Losses: Object containing checkpoint loss, backward loss and additional losses indexed by name.
+            Additional losses includes proximal loss.


No proximal loss in these calculations 🙂

emersodb · 2023-11-20T15:47:29Z

fl4health/clients/moon_client.py

+            Tuple[Dict[str, torch.Tensor], Dict[str, torch.Tensor]]: A tuple in which the first element
+            contains predictions indexed by name and the second element contains intermediate activations
+            index by name. Specificaly the features of the model, features of the global model and features of
+            the old model are passed.


Super minor I would say are returned rather than "passed"

emersodb · 2023-11-20T15:52:34Z

fl4health/model_bases/fenda_base.py

            "local_features": local_output.reshape(len(local_output), -1),
            "global_features": global_output.reshape(len(global_output), -1),
        }
+        # Return preds and features as seperate dictionairy as in moon base


seperate -> separate 🙂

fl4health/utils/metrics.py

emersodb · 2023-11-20T16:05:17Z

fl4health/utils/metrics.py

-    def clear(self) -> None:
-        self.metric_values_history = [[] for _ in range(len(self.metrics))]
-        self.counts = []
+        self.og_metrics = metrics


I'm not sure what the og here stands for unless you meant "original gangster" metrics, which would be funny, but probably not necessary 😂

hahah not quite, its just a short form I have used for original but I can change it to avoid that interpretation

emersodb · 2023-11-20T16:21:42Z

fl4health/utils/metrics.py

+        Args:
+            preds (Dict[str, torch.Tensor]): A dictionairy of
+        """
+        if len(self.metrics_per_prediction_type) == 0:


super minor, but I think you can just do if self. metrics_per_prediction_type:

I don't think this is the case for dictionaries, just verified in python interpreter

huh...I thought I verified it in an interpreter too 😂

>>> dict_empty = {} >>> dict_filled = {"a": "b"} >>> print("HI") if dict_empty else print("BYE") BYE >>> print("HI") if dict_filled else print("BYE") HI

I'm fine with leaving it though. It's very minor

Sorry its cause if self. metrics_per_prediction_type: should be if not self. metrics_per_prediction_type:. My bad hahaha. Updated in my most recent commit.

Right...also my bad for not writing the correct condition 🤦

emersodb · 2023-11-20T16:28:56Z

fl4health/utils/metrics.py


-    def __init__(self, key_to_meter_map: Dict[str, MetricMeter]):
-        self.key_to_meter_map = key_to_meter_map
+        for pred, mtrcs in zip(preds.values(), self.metrics_per_prediction_type.values()):


I'm a bit wary of doing the zip here, as it assumes that the keys of the dictionaries are ordered in the same way and are the same length and won't fail if that is not the case. Maybe we do something like

assert len(preds) == len(self.metrics_per_prediction_type) for prediction_key, pred in prediction_keys.items(): metrics_for_prediction_type = self.metrics_per_prediction_type[prediction_key] for metric_for_prediction_type in metrics_for_prediction_type: metric_for_prediction_type.update(pred, target)

Yeah I agree this is a better way to go about it. Just one small issue, the length of the metric_per_prediction_type is going to as long as as the number of prediction types not the as the number of metrics. Thus I instead assert that the list of metrics at a given key is long as preds.

emersodb · 2023-11-20T16:32:09Z

fl4health/utils/metrics.py

-        for meter in self.key_to_meter_map.values():
-            result = meter.compute()
-            all_results.update(result)
+        for metrics_key, mtrcs in self.metrics_per_prediction_type.items():


Super minor, but let's avoid the abbreviation here, since metrics is pretty short anyway.

emersodb · 2023-11-20T16:33:04Z

fl4health/utils/metrics.py

-        self.metric_values_history = [[] for _ in range(len(self.metrics))]
-        self.counts = []
+        self.og_metrics = metrics
+        self.metric_mngr_name = metric_mngr_name


any objection to expanding these to metric_manager_name and metric_manager_name , respectively since we're only saving three letters in the abbreviation anyway?

There are a few other places in the code where we use this abbreviation that I'd suggest we expand as well, unless you really don't like it 🙂

emersodb

Overall, I think this is an awesome refactor. All the comments I left are quite minor and I didn't see anything major that was missed.

…tions

emersodb · 2023-11-21T14:01:08Z

fl4health/clients/fed_prox_client.py

@@ -98,7 +98,8 @@ def compute_loss(
        self, preds: Dict[str, torch.Tensor], features: Dict[str, torch.Tensor], target: torch.Tensor
    ) -> Losses:
        """
-        Computes loss given predictions of the model and ground truth data.
+        Computes loss given predictions of the model and ground truth data. Adds to objective by including
+        proximal loss which is the L2 norm between the initial and final weights of local training.


I'm going to be a pedantic mathematician and have you lower-case l2 here. L^2/L_2 norms operate on functions not vectors lol

hahahah don't ever not be a pedantic mathematician, its good to know these notation conventions

emersodb

Changes look good to me. Left two additional small comments. Feel free to take them or leave them

jewelltaylor added 5 commits November 3, 2023 13:35

fix pre-commit

05e68b7

Update Metric related classes documentation with new formatting

bb0b0ba

Add more standard documentation of methods that I worked with in this…

fcae1a3

… PR.

Update documentation to FedOpt Custom Metrix

a0d1a9a

jewelltaylor mentioned this pull request Nov 17, 2023

Create Fixed Requirements File for FLamby, Update Dynamic Weight Exchanger and FedOpt Example #68

Merged

jewelltaylor added 3 commits November 17, 2023 15:14

Resolve merge conflict and update new fenda client to use seperate di…

0280e17

…ctionairies for predictions and features, bringing it in line with other methods

Fix broken test as a result of change in output of fenda predict method

b8c0fd7

Change moon base to return tuple of dictionairies representing predic…

6f66a10

…tions and features. Also added some comments throughout

jewelltaylor requested review from emersodb, sanaAyrml, yc7z, fatemetkl and zxj-c November 17, 2023 21:14

Merge branch 'main' into metric-refactor

767d641

emersodb reviewed Nov 20, 2023

View reviewed changes

fl4health/utils/metrics.py Show resolved Hide resolved

emersodb reviewed Nov 20, 2023

View reviewed changes

jewelltaylor added 2 commits November 20, 2023 13:49

Update documentation, variable names and a few other of Davids sugges…

5231ec6

…tions

Simplify check if dict is empty

a20508b

emersodb reviewed Nov 21, 2023

View reviewed changes

emersodb self-requested a review November 21, 2023 14:03

emersodb approved these changes Nov 21, 2023

View reviewed changes

Minor documentation fixes

8ecb8ce

jewelltaylor merged commit bc143bc into main Nov 21, 2023
2 checks passed

jewelltaylor deleted the metric-refactor branch November 21, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metric refactor #69

Metric refactor #69

jewelltaylor commented Nov 4, 2023 •

edited

Loading

jewelltaylor commented Nov 17, 2023

emersodb Nov 20, 2023

jewelltaylor Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023 •

edited

Loading

emersodb Nov 21, 2023

jewelltaylor Nov 21, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

jewelltaylor Nov 20, 2023

emersodb Nov 20, 2023 •

edited

Loading

jewelltaylor Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

jewelltaylor Nov 20, 2023 •

edited

Loading

emersodb Nov 20, 2023

emersodb Nov 20, 2023

jewelltaylor Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb Nov 20, 2023

emersodb left a comment

emersodb Nov 21, 2023

jewelltaylor Nov 21, 2023

emersodb left a comment


		Args:
		label_encoder (LabelEncoder): This class is used to determine the mapping of integers to label names for

Metric refactor #69

Metric refactor #69

Conversation

jewelltaylor commented Nov 4, 2023 • edited Loading

PR Type

Short Description

Tests Added

jewelltaylor commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersodb Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersodb Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jewelltaylor Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersodb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersodb left a comment

Choose a reason for hiding this comment

jewelltaylor commented Nov 4, 2023 •

edited

Loading

emersodb Nov 20, 2023 •

edited

Loading

emersodb Nov 20, 2023 •

edited

Loading

jewelltaylor Nov 20, 2023 •

edited

Loading