Skip to content
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

More presto benchmarking #384

Merged
merged 3 commits into from
Feb 19, 2024
Merged

More presto benchmarking #384

merged 3 commits into from
Feb 19, 2024

Conversation

ivanzvonkov
Copy link
Collaborator

@ivanzvonkov ivanzvonkov commented Feb 6, 2024

Ran benchmark using Presto band group encodings.

Posting Google sheet in slack.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -455,7 +457,7 @@ def forward(
if aggregate == Aggregate.MEAN:
return self.norm(x.mean(dim=1))
elif aggregate == Aggregate.BAND_GROUPS_MEAN:
return self.norm(self.band_groups_mean(x, kept_indices, num_timesteps))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not possible to use self.norm here as it expected embedding_size shape

@@ -358,7 +358,9 @@ def band_groups_mean(
mask = (kept_indices >= min_idx) & (kept_indices < max_idx)
# we assume kept_elements is the same for all batches
kept_elements = sum(mask[0, :])
groups.append(x[mask.bool()].view(batch_size, kept_elements, embedding_dim).mean(dim=1))
one_group = x[mask.bool()].view(batch_size, kept_elements, embedding_dim).mean(dim=1)
one_group_normed = self.norm(one_group)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fix for the self.norm issue. Creating a new LayerNorm was not possible because then it expected a weight and bias for that variable in the pretrained model.

@@ -9,7 +9,7 @@
"\n",
Copy link
Contributor

@gabrieltseng gabrieltseng Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of convergence warnings here; I wonder if its worth increasing the number of iterations just to see.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! This is why I used the StandardScaler as attempt at a solution. I'll try more iterations

@ivanzvonkov ivanzvonkov merged commit 2eba0db into master Feb 19, 2024
2 of 4 checks passed
@ivanzvonkov ivanzvonkov deleted the more-presto-benchmarking branch February 19, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants