-
Notifications
You must be signed in to change notification settings - Fork 6
Feat/vals #346
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.
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
base: main
Are you sure you want to change the base?
Feat/vals #346
Changes from 14 commits
3e79399
2e35b74
15bba55
d015a77
56cad09
1187966
d1acdac
15c75e5
d03871c
971ddac
48e621d
f54b019
0e6d7df
ebe2ec5
62f3d77
df801ef
0b482fc
8692bd2
b67d7ed
b8eef19
233b3a1
d5600b7
dd9d1cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import lightning as L | ||
| import torch | ||
| from torch import nn | ||
| from torch.optim import AdamW | ||
| from torch.optim.lr_scheduler import LinearLR | ||
|
|
@@ -23,18 +24,21 @@ def __init__( | |
| learning_rate: float = 5e-4, | ||
| optimizer_epsilon: float = 1e-6, | ||
| scheduler_warmup_epochs: int = 0, | ||
| value_loss_weight: float = 1.0, | ||
| ): | ||
| super().__init__() | ||
| self.learning_rate = learning_rate | ||
| self.optimizer_epsilon = optimizer_epsilon | ||
| self.scheduler_warmup_epochs = scheduler_warmup_epochs | ||
| self.value_loss_weight = value_loss_weight | ||
|
|
||
| self.model = model | ||
| if compile_mode is not None: | ||
| self.model.compile(mode=compile_mode) | ||
|
|
||
| self.train_loss = loss_types[self.model.hparams["attn_type"]]() | ||
| self.val_loss = nn.CrossEntropyLoss() | ||
| self.value_loss_fn = nn.MSELoss() | ||
| self.val_metrics = self.configure_metrics("val") | ||
|
|
||
| hparams = self.model.hparams.copy() | ||
|
|
@@ -43,6 +47,7 @@ def __init__( | |
| "learning_rate": learning_rate, | ||
| "optimizer_epsilon": optimizer_epsilon, | ||
| "scheduler_warmup_epochs": scheduler_warmup_epochs, | ||
| "value_loss_weight": value_loss_weight, | ||
| } | ||
| ) | ||
| self.save_hyperparameters(hparams) | ||
|
|
@@ -69,16 +74,36 @@ def configure_metrics(self, prefix: str): | |
| ], | ||
| ) | ||
|
|
||
| def compute_combined_loss(self, batch, concept_loss_fn): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be a loss function specific to training with values that that is only instantiated with values so no conditionals are necessary
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think (2) is definitely feasible as that's just what I ended up doing in the original codebase, but would we prefer this? Or something like (1) instead?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (2) is probably the cleanest code-wise, but it's hacky, and (1) is probably the cleanest logic-wise. For this, I would try out (1) first and see if this resolves things. I don't mind having some if statements (e.g. So we could do either:
Hard to tell if this also becomes messy in its own way, but I'm not the biggest fan of (2) unless alternatives don't work
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a bad idea to go forward with a maybe it's there maybe it's not approach. That forces these lines in all functions, which is an eyesore in itself, and it also extends horribly: I think either commit to it always being there, with the option of it being empty (solution 2), or commit to having specific functionality that should be invoked when values are there (solution 1)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I split up the bonsai_nets to value and non-values, but there are still quite a lot of if-statements during data creation. Are we content with that, or would be also like to split this up? I'm not what the most clean way of doing this would be, as it is mostly just a few lines in each function. |
||
| logits, labels, val_logits, val_labels = self.model(batch) | ||
| concept_loss = concept_loss_fn( | ||
| logits.view(-1, logits.size(-1)), labels.view(-1) | ||
| ) | ||
|
|
||
| if val_logits is not None and val_labels is not None: | ||
| value_loss = self.value_loss_fn(val_logits, val_labels) | ||
| else: | ||
| value_loss = torch.zeros((), device=logits.device, dtype=logits.dtype) | ||
|
|
||
| loss = concept_loss + self.value_loss_weight * value_loss | ||
| return loss, concept_loss, value_loss, logits, labels | ||
|
|
||
| def training_step(self, batch, batch_idx): | ||
| logits, labels = self.model(batch) | ||
| loss = self.train_loss(logits.view(-1, logits.size(-1)), labels.view(-1)) | ||
| loss, concept_loss, value_loss, _, _ = self.compute_combined_loss( | ||
| batch, self.train_loss | ||
| ) | ||
| self.log("train/loss", loss, prog_bar=True) | ||
| self.log("train/concept_loss", concept_loss, prog_bar=True) | ||
| self.log("train/value_loss", value_loss, prog_bar=True) | ||
| return loss | ||
|
|
||
| def validation_step(self, batch, batch_idx): | ||
| logits, labels = self.model(batch) | ||
| loss = self.val_loss(logits.view(-1, logits.size(-1)), labels.view(-1)) | ||
| loss, concept_loss, value_loss, logits, labels = self.compute_combined_loss( | ||
| batch, self.val_loss | ||
| ) | ||
| self.log("val/loss", loss, prog_bar=True) | ||
| self.log("val/concept_loss", concept_loss, prog_bar=True) | ||
| self.log("val/value_loss", value_loss, prog_bar=True) | ||
| self.val_metrics.update(logits, labels) | ||
| self.log_dict(self.val_metrics) | ||
| return loss | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.