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

Cebra #256

Merged
merged 15 commits into from
Aug 23, 2023
Merged

Cebra #256

merged 15 commits into from
Aug 23, 2023

Conversation

katrinaager
Copy link
Collaborator

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:23Z
----------------------------------------------------------------

This should be changed to use the latest environment setup standard. Look at notebooks on dev for examples. Make sure to include the bolded warning, and move import os to a following cell.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:24Z
----------------------------------------------------------------

shouldn't be pip installing in the notebook. If CEBRA is iin requirements.txt it should be installed from the environment setup cell.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:27Z
----------------------------------------------------------------

Line #9.    from dandi_utils import dandi_download_open

once the environment setup cell is updated, remove this line


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:28Z
----------------------------------------------------------------

Be clear when you say "retrieves particular data", be more specific. What is the data?


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:29Z
----------------------------------------------------------------

You already explain dff_timestamps and dff_traces in section above. Maybe reduce the redundancy between these two?

Extra space in "properly index"

"will be input" should be "will be inputted"

when you say "can properly index and label the neural data" dont say neural data! Everything is neural data. what is it? Could say something like "can properly index and label the fluorescence measurements"


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:30Z
----------------------------------------------------------------

It looks like all three of these arrays are intended to hold the same number of elements which correspond to eachother? Would be better practice to store all of these together as a list of 3-tuples rather than three separate lists. This would also help to understand the purpose of these lists, which isn't immediately clear from looking at it.

Would help to have a comment when you define the empty list of tuples explaining what goes in it.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:31Z
----------------------------------------------------------------

Same thing here as above, if these two lists contain corresponding elements then a list of tuples is probably clearer.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:32Z
----------------------------------------------------------------

good explanation

typo in "if we try to index a timestamps" (extra S)


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:33Z
----------------------------------------------------------------

great


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:34Z
----------------------------------------------------------------

replace "neural data" with something more specific.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:35Z
----------------------------------------------------------------

Line #1.    # insert the data you want to train in .fit()

replace "data" with something more specific


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:36Z
----------------------------------------------------------------

if possible, explain what "low" means in this case. "You should see a low loss value such as __"


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:37Z
----------------------------------------------------------------

Line #1.    ### inputs neural data for each repeat of the stimulus into the model and gets colors for embedding space

replace "neural data" with something more specific

also specify what this function returns, that's most important. It returns the embedding space of points and the color labels for each point.


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:37Z
----------------------------------------------------------------

Line #4.        # takes neural data for a particular stimulus and selects only data for one specific repeat

and here too


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:38Z
----------------------------------------------------------------

Line #13.        # takes frames of visual movie for a particular stimulus and selectes only frames from a specific repeat of the movie

tYPO


@review-notebook-app
Copy link

review-notebook-app bot commented Aug 8, 2023

View / edit / reply to this conversation on ReviewNB

rcpeene commented on 2023-08-08T18:11:39Z
----------------------------------------------------------------

this should def be broken up into a sub function. too many loops within loops


@jeromelecoq
Copy link
Collaborator

@rcpeene and @katrinaager can you review the change. I added the grid search section.

@rcpeene rcpeene changed the title cebra code review Cebra Aug 16, 2023
@katrinaager
Copy link
Collaborator Author

The new embedding spaces look good! they seem to be following a more track-like pattern.

@rcpeene rcpeene merged commit a43ac54 into dev Aug 23, 2023
1 check passed
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.

3 participants