Hi, I was reading through this S-CGIB implementation (which i think is the official one) and had a few questions about some parts of the code. I may be misunderstanding something, so I wanted to ask for clarification.
-
Possible bug in KL_tensor_all accumulation
In models.py, inside compression(), I noticed:
KL_tensor_all = torch.cat((KL_tensor, KL_tensor), 0)
Should this be concatenating with the running accumulator instead? As written, it seems to overwrite the previous contents and duplicate only the current graph’s KL_tensor, so the final tensor would reflect only the last graph in the batch rather than all graphs.
-
Adjacency reconstruction seems different from the paper
The paper defines adjacency reconstruction using cosine similarity between final node embeddings and scales the loss by (1/|V|^2). In the code, however, loss_recon_adj() does:
def loss_recon_adj(self, interaction_map, org_graph, batch_size=16):
row_num, col_num = interaction_map.size()
adj = org_graph.adj().to_dense()
recon_interaction_map = torch.mm(interaction_map, interaction_map.t())
loss = torch.sum((recon_interaction_map - adj) ** 2) / (row_num)
return loss
This seems to use a plain inner product instead of cosine similarity, and divides by row_num rather than by (|V|^2). I wanted to ask whether this difference from Eq. (16) is intentional.
-
Domain adaptation loss seems graph-level rather than node-level
From the paper, I understood the domain adaptation loss as reconstructing each node feature (x_i) from its corresponding final representation \tilde{H}_i. In Mainmodel_domainadapt.forward(), however, the code appears to:
- call the pretrained feature extractor,
- apply
self.MLP(interaction_map),
- pool over the whole graph with
Set2Set,
- project the pooled vector with
r_transfer_d,
- compare it against a pooled version of the original node features using
s2s_rev,
- compute squared error on those graph-level pooled vectors.
So it looks more like graph-level pooled reconstruction than node-level feature reconstruction. Is that the intended implementation of Eq. (20), or is it a different variant?
-
MLP before reconstruction
The code applies an MLP before reconstruction, whereas Eq. (16) in the paper reconstructs from \tilde{H} directly. Also, in the appendix, if I understood correctly, the main reported variant uses adjacency reconstruction without FC decoder layers, while FC+Adj appears as an ablation. I wanted to ask whether the current implementation intentionally uses that ablation-style variant.
-
(Important) Possible issue with nested models and self.model.extract_features()
This is the part I am most unsure about, but it seems important.
Several classes such as Mainmodel_continue, Mainmodel_domainadapt, and Mainmodel_finetuning call self.model.extract_features(...), where self.model is itself a previously saved model loaded from disk.
My concern is that this may create a mismatch between:
- the encoder modules defined on the current wrapper class instance, and
- the encoder modules actually used in forward().
For example:
- During domain adaptation, a
Mainmodel_domainadapt.forward() uses self.model.extract_features so the active encoders are those inside the loaded pretrained model
- after saving that domain-adapted object and loading it inside
Mainmodel_finetuning, forward() again calls self.model.extract_features(), which now seems to use the encoder stack belonging to the loaded wrapper object rather than the encoder stack that was active during the previous stage.
A similar issue may happen in Mainmodel_continue during multi-stage pretraining in exp_pretraining.py, where a saved Mainmodel_continue is loaded into another Mainmodel_continue.
So my question is: is this nesting intentional? I am worried that, across stages, the encoder actually used in forward() may not be the same encoder that was trained in the previous stage.
Thank you very much for your time. I would really appreciate any clarification on these points.
Hi, I was reading through this S-CGIB implementation (which i think is the official one) and had a few questions about some parts of the code. I may be misunderstanding something, so I wanted to ask for clarification.
Possible bug in KL_tensor_all accumulation
In models.py, inside compression(), I noticed:
KL_tensor_all = torch.cat((KL_tensor, KL_tensor), 0)Should this be concatenating with the running accumulator instead? As written, it seems to overwrite the previous contents and duplicate only the current graph’s
KL_tensor, so the final tensor would reflect only the last graph in the batch rather than all graphs.Adjacency reconstruction seems different from the paper
The paper defines adjacency reconstruction using cosine similarity between final node embeddings and scales the loss by (1/|V|^2). In the code, however,
loss_recon_adj()does:def loss_recon_adj(self, interaction_map, org_graph, batch_size=16):row_num, col_num = interaction_map.size()adj = org_graph.adj().to_dense()recon_interaction_map = torch.mm(interaction_map, interaction_map.t())loss = torch.sum((recon_interaction_map - adj) ** 2) / (row_num)return lossThis seems to use a plain inner product instead of cosine similarity, and divides by row_num rather than by (|V|^2). I wanted to ask whether this difference from Eq. (16) is intentional.
Domain adaptation loss seems graph-level rather than node-level
From the paper, I understood the domain adaptation loss as reconstructing each node feature (x_i) from its corresponding final representation \tilde{H}_i. In
Mainmodel_domainadapt.forward(), however, the code appears to:self.MLP(interaction_map),Set2Set,r_transfer_d,s2s_rev,So it looks more like graph-level pooled reconstruction than node-level feature reconstruction. Is that the intended implementation of Eq. (20), or is it a different variant?
MLP before reconstruction
The code applies an MLP before reconstruction, whereas Eq. (16) in the paper reconstructs from \tilde{H} directly. Also, in the appendix, if I understood correctly, the main reported variant uses adjacency reconstruction without FC decoder layers, while FC+Adj appears as an ablation. I wanted to ask whether the current implementation intentionally uses that ablation-style variant.
(Important) Possible issue with nested models and self.model.extract_features()
This is the part I am most unsure about, but it seems important.
Several classes such as
Mainmodel_continue,Mainmodel_domainadapt, andMainmodel_finetuningcallself.model.extract_features(...), whereself.modelis itself a previously saved model loaded from disk.My concern is that this may create a mismatch between:
For example:
Mainmodel_domainadapt.forward()usesself.model.extract_featuresso the active encoders are those inside the loaded pretrained modelMainmodel_finetuning,forward()again callsself.model.extract_features(), which now seems to use the encoder stack belonging to the loaded wrapper object rather than the encoder stack that was active during the previous stage.A similar issue may happen in
Mainmodel_continueduring multi-stage pretraining inexp_pretraining.py, where a savedMainmodel_continueis loaded into anotherMainmodel_continue.So my question is: is this nesting intentional? I am worried that, across stages, the encoder actually used in
forward()may not be the same encoder that was trained in the previous stage.Thank you very much for your time. I would really appreciate any clarification on these points.