Closed ivirshup closed 8 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:26Z ----------------------------------------------------------------
Line #4. ) # "MT-" for human, "Mt-" for mouse
please move the comment above the line for better formatting
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:26Z ----------------------------------------------------------------
Is there a citation for the “permissive filtering” insight? If everything is explained in luecken2021
, maybe mention that.
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:27Z ----------------------------------------------------------------
Looks like quite some overplotting. Maybe mention that this is not an ideal representation of the density of the samples?
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:28Z ----------------------------------------------------------------
We throw a deprecation warning when not using it, right? Maybe mention that scanpy recommends using it.
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:29Z ----------------------------------------------------------------
Why cluster before this?
ivirshup commented on 2024-03-22T21:25:14Z ----------------------------------------------------------------
Largely because I don't want to vary too much from the main notebook over on scverse-tutorials.
Tbh we could probably just remove this block, and suggest that the process should be iterative and maybe clusters should be removed.
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:30Z ----------------------------------------------------------------
here too: move the comments above their dict entries to give them a chance to go on one line. (make sure you re-format with the last comma per list removed so the formatter doesn’t keep it on multiple lines)
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:30Z ----------------------------------------------------------------
This part is confusing. You redefine the cluster3_genes
variable, the names in this text cell don’t correspond to the names the code emits, please check this part again
View / edit / reply to this conversation on ReviewNB
flying-sheep commented on 2024-03-22T10:33:31Z ----------------------------------------------------------------
this is not a violin plot as advertised
Largely because I don't want to vary too much from the main notebook over on scverse-tutorials.
Tbh we could probably just remove this block, and suggest that the process should be iterative and maybe clusters should be removed.
View entire conversation on ReviewNB
I believe I've addressed everything and want to get moving on the release. Open follow up issues for anything else.
Making a doc update for scanpy 1.10. Includes some changes for this to better support being a gitsubmodule, though also makes a minor updates to docs.
see https://github.com/scverse/scanpy/pull/2901