search

natverse / fafbseg

Support functions for analysis of Drosophila connectomes especially the FAFB-FlyWire whole brain
https://natverse.org/fafbseg/
GNU General Public License v3.0
6 stars 4 forks source link

Flywire based changes #22

Closed SridharJagannathan closed 4 years ago

SridharJagannathan commented 4 years ago

This is an ongoing PR, it contains: 1) Added documentation about graphene server and how to access it.

SridharJagannathan commented 4 years ago

@jefferis : Can you please merge your changes in the branch feature/flywire-meshes to the master, I have already synced my changes to your branch, I have also added some mock testcases and made some small changes to your implementation.

jefferis commented 4 years ago

Hi @SridharJagannathan, thank you for all of this. A few and/thoughts. Was it essential to merge in my changes to resolve conflicts? Although I guess git blame should still report who wrote each line, I find it quite difficult to review this PR now because it includes a bunch of things that you wrote and bunch that I did. Also some of my work (eg dracor/graphene meshes) was still WIP — I was deciding if/how to handle it, so I would rather it wasn’t bundled in here.

As a general comment, I believe that review time and complexity scale supralinearly with the amount of changes involved. Therefore I’d rather receive 2 or 3 more focussed PRs than one huge one covering a whole range of areas.

Would it be possible to remove my changes for now? Or did you need to so much work to merge them?

SridharJagannathan commented 4 years ago

Sure, I wasn't actually going to add your changes until I found that one of the file that i touched flywire-fetch had the function read_graphene_meshes moved to another file, only the last commit contains your changes merged, I can remove them again and then you can check it. Actually with regards to the number of changes, the main changes are only in flywire-fetch the other enormous change you see is mainly files added due to the mocking of the http based testcases. Verification of the changes rolled back: https://github.com/natverse/fafbseg/compare/cf0d4fb..340bfa4