68 replace month pe#71
Conversation
… learend projection into embedding space is used. removes PE
meiertgrootes
requested review from
SarahAlidoost and
rogerkuou
and removed request for
SarahAlidoost
| months. | ||
| """ | ||
|
|
||
| def __init__(self, embed_dim=128, max_months=12, dropout=0.0): |
There was a problem hiding this comment.
| def __init__(self, embed_dim=128, dropout=0.0): |
There was a problem hiding this comment.
removing leftover max_months=12,
| month_emb = (token_emb_seq * day_w).sum(dim=3) | ||
|
|
||
| month_tokens = month_tokens + month_emb |
There was a problem hiding this comment.
| aggregated_month_embed = (token_emb_seq * day_w).sum(dim=3) | |
| month_tokens = month_tokens + aggregated_month_embed |
There was a problem hiding this comment.
because the variable name month_emb is used for something else in previous lines. Here I suggested to rename it in this line.
| T: number of temporal tokens per month after temporal patching (Tp) | ||
| H: spatial height after spatial patching | ||
| W: spatial width after spatial patching | ||
| time_features: (B,M,T,2) containing cyclically phase encoded DOY and HOD |
There was a problem hiding this comment.
| time_features: (B,M,T,3) containing cyclically phase encoded MOY, DOY and HOD |
| @@ -629,14 +630,14 @@ | |||
| embed_dim=128, | |||
| patch_size=(1, 4, 4), | |||
| max_months=12, | |||
There was a problem hiding this comment.
| max_months=12, |
| @@ -645,7 +646,6 @@ | |||
| embed_dim: Dimension of the patch embedding | |||
| patch_size: Tuple of (T, H, W) patch sizes for temporal and spatial patching | |||
| max_months: Maximum number of months for temporal positional encoding | |||
There was a problem hiding this comment.
| max_months: Maximum number of months for temporal positional encoding |
| embed_dim=embed_dim, | ||
| max_months=max_months, |
There was a problem hiding this comment.
can you please remove max_months=max_months,. I couldnot add it as code suggestion.
SarahAlidoost
left a comment
SarahAlidoost
left a comment
There was a problem hiding this comment.
@meiertgrootes thanks, nice implementation and improvement! 🥇 I could run the notebook without any issue. I left some minor comments. This is ready to merge. I noticed the branch 68_replace_month_pe is based on branch in #64 . Let's merge #64 to main first before merging this to main.
2 participants
This PR replace the PE based month encoding which was based off of sequence position with a time stamp based cyclical phase encoding of the month, creating a projected embedding from a C12 group fourier base embedding.
Note @SarahAlidoost @rogerkuou , as Levante is down and I do not have the appropriate data files locally I HAVE NOT been able to run the example note books.
Please consider doing so during review. Should errors occur feel free to just drop it back to me