68 replace month pe - #71
Conversation
… learend projection into embedding space is used. removes PE
There was a problem hiding this comment.
| time_features: (B,M,T,3) containing cyclically phase encoded MOY, DOY and HOD |
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.
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
|
@SarahAlidoost @rogerkuou I've addressed @SarahAlidoost 's comments. I've also reviewed #64 which is ready to be merged imo. So ready to merge this PR after #64 is merged, pending @rogerkuou 's review |
There was a problem hiding this comment.
Hi @meiertgrootes , I do not have further comments! Please go ahead and merge. Thanks for the implementation!
3 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