-
Notifications
You must be signed in to change notification settings - Fork 44
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
Added shifter and updated 3d-encoder nonlinearity #230
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
========================================
- Coverage 7.52% 7.51% -0.02%
========================================
Files 58 58
Lines 5965 5978 +13
Branches 1013 1018 +5
========================================
Hits 449 449
- Misses 5479 5492 +13
Partials 37 37 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I compared to https://github.com/ecker-lab/sensorium_2023/blob/main/sensorium/models/video_encoder.py but am not very much experienced with the shifters; I trust you here ;)
pupil_center = pupil_center[:, :, -time_points:] | ||
pupil_center = torch.transpose(pupil_center, 1, 2) | ||
pupil_center = pupil_center.reshape(((-1,) + pupil_center.size()[2:])) | ||
shift = self.shifter[data_key](pupil_center, trial_idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename shift
into shifter
? From how the word sounds, I would expect shift
to be bool
, but it actually is rather smth like a nn.Module
I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok just seen that in the readouts it's also called shift
, so no strong opinion here from my side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I guess shift
and shifter
are also slightly semantically different
A I get it shift
if the output of the shifter
and theoretically it could be provided for the readout from somewhere else, not from the model.shifter
(that the only reason why this parameter is here and I also tried to make it consistency with the 2d core)
Co-authored-by: Max Burg <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
#227
Plus I changed the non-linearity to be consistent with the 2d firing rate encoder
Shifter reshapes are used as in sensorium 2023