Support validation set and FedEM for MF datasets #310

yxdyc · 2022-08-10T10:13:19Z

as the title says. Please double check the modifications related to MF. Thanks @rayrayraykk @DavdGao

DavdGao

Please see the inline comments

DavdGao · 2022-08-10T11:41:38Z

federatedscope/core/trainers/trainer_FedEM.py

+        """
+            Ensemble evaluation for matrix factorization model
+        """
+        cur_data = ctx.cur_mode


Please ensure that the usage of cur_mode is correct here.

cur_mode: the type of our routine, chosen from "train"/"test"/"val"/"finetune"

cur_split: the chosen data split
Besides, do we still need to name the variables with cur_data, since they are all removed at the end of the routine.

fixed, here we should use cur_split

DavdGao · 2022-08-10T11:45:47Z

federatedscope/core/trainers/trainer_FedEM.py

+            # set the eval_metrics
+            if ctx.num_samples == 0:
+                results = {
+                    f"{cur_data}_avg_loss": ctx.get(


The metric calculator uses cur_split instead, please check if it's correct to use cur_data(actually cur_mode)

fixed as above replied

DavdGao · 2022-08-10T11:47:09Z

federatedscope/core/trainers/trainer_FedEM.py

+                }
+            else:
+                results = {
+                    f"{ctx.cur_mode}_avg_loss": ctx.get(


it's a little confused to use ctx.cur_mode here, since we use cur_data in line 236.

fixed accordingly

DavdGao · 2022-08-10T11:53:07Z

federatedscope/mf/dataset/movielens.py

+        else:
+            self._split_n_clients_rating_vmf(ratings, num_client, split)
+
+    def _split_n_clients_rating_hmf(self, ratings: csc_matrix, num_client: int,


Since the class HMFDataset and VMFDataset also have the function _split_n_clients_rating for HMF and VMF resepectively, maybe we don't need the functions _split_n_clients_rating_hmf and _split_n_clients_rating_vmf here?

deleted it in the new pr

DavdGao · 2022-08-10T11:53:17Z

federatedscope/mf/dataset/movielens.py

+            }
+        self.data = data
+
+    def _split_n_clients_rating_vmf(self, ratings: csc_matrix, num_client: int,


The same as above

deleted it in the new pr

DavdGao · 2022-08-10T11:55:03Z

federatedscope/mf/model/model.py

@@ -45,7 +45,8 @@ def forward(self, indices, ratings):
                                       device=pred.device,
                                       dtype=torch.float32).to_dense()

-        return mask * pred, label, float(np.prod(pred.size())) / len(ratings)
+        return mask * pred, label, torch.Tensor(


Why do we convert it to a Tensor, and do we need to consider the device of the Tensor?

Here the conversion is for flop counting. The device is not important since after counting the flop, the tensor will be discarded.

DavdGao · 2022-08-10T11:58:12Z

federatedscope/mf/trainer/trainer.py

+        if ctx.get("num_samples") == 0:
+            results = {
+                f"{ctx.cur_mode}_avg_loss": ctx.get(
+                    "loss_batch_total_{}".format(ctx.cur_mode)),


It's a little confused that in line 53, we use loss_batch_total_{ctx.cur_mode}, while in line 58 it is ctx.loss_batch_total

changed into loss_batch_total_{ctx.cur_mode} in line 58

DavdGao · 2022-08-10T12:01:06Z

federatedscope/mf/trainer/trainer.py

@@ -66,6 +82,13 @@ def _hook_on_batch_end(self, ctx):
        ctx.loss_batch_total += ctx.loss_batch.item() * ctx.batch_size
        ctx.loss_regular_total += float(ctx.get("loss_regular", 0.))

+        if self.cfg.federate.method.lower() in ["fedem"]:
+            # cache label for evaluation ensemble
+            ctx.get("{}_y_true".format(ctx.cur_mode)).append(


The attribute y_true is a matrix here and can be very large for MF dataset, I'm not sure it's appropriate to storage all the labels and probs

The appended one is sparse csr_matrix

rayrayraykk · 2022-08-11T07:56:08Z

federatedscope/mf/dataset/movielens.py

@@ -18,16 +18,20 @@ class VMFDataset:

    """
    def _split_n_clients_rating(self, ratings: csc_matrix, num_client: int,
-                                test_portion: float):
+                                split: list):


How about enabling this change to FedNetflix?

FedNetflix is inherited from MovieLensData, thus this change should be valid to FedNetflix

yxdyc

modified according to the comments

yxdyc · 2022-10-11T06:55:36Z

federatedscope/core/trainers/trainer_FedEM.py

+        """
+            Ensemble evaluation for matrix factorization model
+        """
+        cur_data = ctx.cur_mode


fixed, here we should use cur_split

yxdyc · 2022-10-11T06:55:58Z

federatedscope/core/trainers/trainer_FedEM.py

+            # set the eval_metrics
+            if ctx.num_samples == 0:
+                results = {
+                    f"{cur_data}_avg_loss": ctx.get(


fixed as above replied

yxdyc · 2022-10-11T06:58:50Z

federatedscope/mf/model/model.py

@@ -45,7 +45,8 @@ def forward(self, indices, ratings):
                                       device=pred.device,
                                       dtype=torch.float32).to_dense()

-        return mask * pred, label, float(np.prod(pred.size())) / len(ratings)
+        return mask * pred, label, torch.Tensor(


Here the conversion is for flop counting. The device is not important since after counting the flop, the tensor will be discarded.

yxdyc · 2022-10-11T07:01:32Z

federatedscope/mf/trainer/trainer.py

@@ -66,6 +82,13 @@ def _hook_on_batch_end(self, ctx):
        ctx.loss_batch_total += ctx.loss_batch.item() * ctx.batch_size
        ctx.loss_regular_total += float(ctx.get("loss_regular", 0.))

+        if self.cfg.federate.method.lower() in ["fedem"]:
+            # cache label for evaluation ensemble
+            ctx.get("{}_y_true".format(ctx.cur_mode)).append(


The appended one is sparse csr_matrix

yxdyc · 2022-10-11T07:11:26Z

federatedscope/core/trainers/trainer_FedEM.py

+                }
+            else:
+                results = {
+                    f"{ctx.cur_mode}_avg_loss": ctx.get(


fixed accordingly

yxdyc · 2022-10-11T07:14:57Z

federatedscope/mf/trainer/trainer.py

+        if ctx.get("num_samples") == 0:
+            results = {
+                f"{ctx.cur_mode}_avg_loss": ctx.get(
+                    "loss_batch_total_{}".format(ctx.cur_mode)),


changed into loss_batch_total_{ctx.cur_mode} in line 58

yxdyc · 2022-10-11T08:36:26Z

federatedscope/mf/dataset/movielens.py

@@ -18,16 +18,20 @@ class VMFDataset:

    """
    def _split_n_clients_rating(self, ratings: csc_matrix, num_client: int,
-                                test_portion: float):
+                                split: list):


FedNetflix is inherited from MovieLensData, thus this change should be valid to FedNetflix

yxdyc · 2022-10-11T08:52:51Z

federatedscope/mf/dataset/movielens.py

+        else:
+            self._split_n_clients_rating_vmf(ratings, num_client, split)
+
+    def _split_n_clients_rating_hmf(self, ratings: csc_matrix, num_client: int,


deleted it in the new pr

yxdyc · 2022-10-11T08:52:54Z

federatedscope/mf/dataset/movielens.py

+            }
+        self.data = data
+
+    def _split_n_clients_rating_vmf(self, ratings: csc_matrix, num_client: int,


deleted it in the new pr

support validation set for MF datasets; fix FedEM for MF datasets;

8d97ffa

yxdyc added the enhancement New feature or request label Aug 10, 2022

yxdyc requested review from DavdGao and rayrayraykk August 10, 2022 11:30

DavdGao reviewed Aug 10, 2022

View reviewed changes

rayrayraykk reviewed Aug 11, 2022

View reviewed changes

yxdyc added 2 commits October 11, 2022 16:47

modified according to david's comment

3c67196

modified according to david's comment

2297679

yxdyc commented Oct 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support validation set and FedEM for MF datasets #310

Support validation set and FedEM for MF datasets #310

yxdyc commented Aug 10, 2022 •

edited

DavdGao left a comment

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

DavdGao Aug 10, 2022

yxdyc Oct 11, 2022

rayrayraykk Aug 11, 2022

yxdyc Oct 11, 2022

yxdyc left a comment

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

yxdyc Oct 11, 2022

Support validation set and FedEM for MF datasets #310

Are you sure you want to change the base?

Support validation set and FedEM for MF datasets #310

Conversation

yxdyc commented Aug 10, 2022 • edited

DavdGao left a 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

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

yxdyc left a 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

yxdyc commented Aug 10, 2022 •

edited