Improve repair #65
Improve repair #65
Conversation
arnaudon
force-pushed
the
improve_repair
branch
5 times, most recently
from
9d3c7d8
to
c7fc832
Compare
|
@wizmer , good to review. Once we agree on the code, I'll update the tests. They may change a bit if we change things. |
arnaudon
force-pushed
the
improve_repair
branch
from
ee2fb7b
to
5a1d16e
Compare
arnaudon
force-pushed
the
improve_repair
branch
from
d57f2ca
to
df8bac2
Compare
|
@asanin-epfl @adrien-berchet , you can now review this! Thanks! |
| @@ -147,27 +150,6 @@ def _last_segment_vector(section, normalized=False): | |||
| return vec | |||
|
|
|||
|
|
|||
| def _get_similar_child_diameters(sections, original_section): | |||
There was a problem hiding this comment.
I hope it's been validated against repair-workflow.
There was a problem hiding this comment.
nope, I will have to pin the neuror version in repair-workflow for the time being. But MCAR (the newest workflow) will do the same, so it will be duplicated calls to repair. Because of all the new parameters we can access now, I will only maintain MCAR (in cells/morphology-processing-workflow).
There was a problem hiding this comment.
What is used for the release of morphologies? MCAR or repair-workflow? If we merge this, and repair-workflow fails, what to do then? This can't be patched at the level of repair-workflow. Pinning of NeuroR is a shaky solution. If any patch comes to NeuroR later, how to apply it? The version is pinned.
There was a problem hiding this comment.
yes, we need to be careful here. I will run tests of morphology-repair-workflow to be sure the damage is not too big. This particular function I removed as I don't use it internally, but it will not be visible from outisde. This function was not doing the correct thing with diameters. We know diameters should decrease, and this function my not respect it. It is function benoit copied from a very old code, but I worked on diameters quite a lot, and we can do better than that.
There was a problem hiding this comment.
It is function benoit copied from a very old code, but I worked on diameters quite a lot, and we can do better than that.
It's not about being better. The main problem is compatibility with legacy. From what I understood, we must reproduce the same results as the old code. That is why there is such arguments as legacy_detection etc.
There was a problem hiding this comment.
if you really don't want to merge it here, I'll copy this code over to mine.
There was a problem hiding this comment.
Benoit's version was legacy, and it was ok.
From what I understand there are 2 legacy versions. The first is when Benoit rewrote for the first time. The second is when it turned out that the first produces different results than the original legacy by RepairSDK.
if you really don't want to merge it here
It's not about mine desire. It's about we don't break repair-workflow. If you don't use it for release of morphologies then I'm ok.
There was a problem hiding this comment.
Yes, I know your point. The legacy version is for cut plane, not for this repair function. We can wait the merge until I have compared results with previous version and morphometric on the tickets. I wanted to do that but neurom didn't... Now you have merge the other 2 PR, I'll retry!
There was a problem hiding this comment.
Thank you. As soon as you confirm repair-workflow, we can merge here.
P.S. CI is still required.
There was a problem hiding this comment.
CI is driving me crazy!
adrien-berchet
force-pushed
the
improve_repair
branch
from
30acbd0
to
66ba998
Compare
|
Thanks for squashing all this mess, Adrien! |
Work PR to improve repair algorithm. Current modifications: