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

Review: suggestions for "First-order analysis" section #397

Merged
merged 10 commits into from
Jun 18, 2024

Conversation

stephprince
Copy link
Collaborator

Here are my suggestions for the first-order analysis section! I think if I open the PR with these changes first, I should be able to make more general suggestions in the review notebook app. I will try to add them there, but let me know if you prefer I make an issue like I did for the other sections.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -6,7 +6,7 @@
"source": [
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Where does this electrode validity metric come from? Could you explain what it means to be invalid?


Reply via ReviewNB

@@ -6,7 +6,7 @@
"source": [
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

I think the threshold for FS vs RS cells in mice is usually around 0.4 ms (can vary depending on the exact waveform duration calculation and brain region). I'm wondering if you want to add that or a citation, right now the text sort of implies the threshold should go anywhere between the bimodal peaks that has the fewest units, which might not always be a "good" threshold (e.g. if there is not enough data for that session or it is not great quality).


Reply via ReviewNB

@@ -6,7 +6,7 @@
"source": [
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Line #10.    ax.plot(time_axis, selected_peak_waveform_fs, color='k', alpha=0.1)

I felt it was difficult to see the individual traces so I modified the transparencies and added an average, but feel free to revert if you prefer the original.


Reply via ReviewNB

@@ -6,7 +6,7 @@
"source": [
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Line #14.    ax.set_ylabel("spike amplitude (uV)")

Since this was extracellular data, I think the y-axis should be spike amplitude or just voltage (vs. membrane potential like it was before).


Reply via ReviewNB

@@ -550,7 +550,7 @@
"metadata": {},
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Line #11.    # interp_lfp = np.transpose(interp_channels)

interp_channels is not defined in the notebook, I suggest adding a comment for how to define and when this would be used, otherwise remove.


Reply via ReviewNB

@@ -7,7 +7,7 @@
"metadata": {},
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Here, certain metrics are calculated for each unit based on their spiking behaviors in response to the optogenetic stimulus.

I think it could be made more clear here that optotagging is the process of identifying these neurons with something like "Here, to determine whether a unit is Cre+ and expresses channelrhodopsin, certain metrics..." But you would need to briefly mention channelrhodopsin at the beginning to add this


Reply via ReviewNB

@@ -7,7 +7,7 @@
"metadata": {},
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

For the metrics you provide it would be helpful to indicate whether higher or lower numbers indicate a greater likelihood of being a Cre+ unit or not


Reply via ReviewNB

@@ -9,7 +9,7 @@
"# Showing Receptive Fields\n",
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

It might be helpful to add why you use Gabor patches for receptive field mapping here. (e.g. something like "Gabor patches are used to reliably drive neural responses.")


Reply via ReviewNB

@@ -7,7 +7,7 @@
"metadata": {},
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

For our dataset, this criterion is rather stringent, only including 15 rois in the final selection.

For each of the inclusion criteria, the number stated in the markdown text and what is displayed in the notebook is different, I changed to keep consistent, but I'm not sure if a different dataset was supposed to be used instead?


Reply via ReviewNB

@@ -7,7 +7,7 @@
"metadata": {},
Copy link
Collaborator Author

@stephprince stephprince May 28, 2024

Choose a reason for hiding this comment

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

Line #1.    roi = 21

I recommend a different example that what was here before (e.g. 21) for baseline vs. evoked response to give the user a clearer idea of what to be looking for.


Reply via ReviewNB

@rcpeene rcpeene changed the base branch from main to dev June 12, 2024 18:08
@rcpeene
Copy link
Collaborator

rcpeene commented Jun 12, 2024

These comments have all been addressed now and pushed to the scientific_review branch,
In order to get it pass the build, could you pull the latest commits from scientific_review?

@stephprince
Copy link
Collaborator Author

@rcpeene Just merged the latest commits from scientific_review into this branch - let me know if you meant something else

@rcpeene rcpeene merged commit 56114be into dev Jun 18, 2024
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.

2 participants