janelia-flyem / gala

Automatic segmentation of electron microscopy volumes
BSD 3-Clause "New" or "Revised" License
75 stars 29 forks source link

`_learn_agglomerate` doesn't use learning mode #63

Closed jni closed 8 years ago

jni commented 8 years ago

The line:

if learning_mode != 'strict' or label < 0:

assumes two learning_mode values exist, "strict" and "loose", but instead the labeling_mode ("assignment") is being passed. This results in implicitly using the "loose" mode, and some downstream indexing errors.

See this mailing list post for more information.

elhuhdron commented 8 years ago

Hi Juan,

Sorry, took me a bit to remove local dependencies I had for loading our data. I've tried to make it simple and without additional dependencies with everything stored in a single h5 file and reads using h5py directly.

I forked gala and added a minimal test set of 64x64x64 voxels under tests/example-data. The test script example_gala_em_briggman.py should run without any additional dependencies besides those needed for gala with python 2.7 (I have not verified with python 3). All the necessary data is contained in a single h5 (with compression enabled), including raw EM data (for reference), supervoxels (watershed), ground truth and probabilities.

The script contains the diff of the fix for gala/agglo.py that then works without error using learning_mode='strict' argument to learn_agglomerate. The same error as before the fix still occurs when using learning_mode='loose'.

Thanks!

jni commented 8 years ago

Well, looks like I introduced this bug on Nov 14, 2011: (!!!)

commit 1db635e963ec95312d9342e2563b1ba21470815e
Author: Juan Nunez-Iglesias <jni@janelia.hhmi.org>
Date:   Mon Nov 14 18:20:01 2011 -0500

    Remove code to compute boundary overlap labels.

    Line profiling showed about 6.5% of time spent on this, for no
    value added in EM contexts.

If you do git diff 1db63^..1db63, you can see that I removed an argument in the call to _learn_agglomerate but didn't remove it from the method def. Mind-boggling that this could have worked for so long undetected.

I'll have a closer look at why there's an error sometimes with 'loose'.

On an unrelated note, have you considered accepting Python 3 into your life? =P

elhuhdron commented 8 years ago
I'll have a closer look at why there's an error sometimes with 'loose'.

Cool, thanks! You'll see when you load the data, but so you're aware, the data has larger areas of extracellular space preserved than in a typical EM prep. All of this area is labeled in the GT with a single value of 1 even if portions of it are not connected. I thought this might be an issue and so I tried (1) re-labeling it, so there were not any non-adjacent labels with the same value in the GT and (2) setting it to background along with membranes (zero) but the error remained in both cases (barring an error in this process on my part).

On an unrelated note, have you considered accepting Python 3 into your life? =P

I'm a perpetual late technology adopter, but soon... :)

jni commented 8 years ago

Ok, for those itching for a fix (it's coming over the official channels, but only after adding many tests): the problem was that when I make the contingency table I predict at most n merges, where n is the number of superpixels. This would be fine except that the merge indices start at n+2, because n+1 is the ID of the "boundary" label, which pads the entire volume.

The options are:

If you have any insights on your own data about whether 'strict' or 'loose' gives better results, please share them here — it would be useful to know in order to evaluate how to deal with published results that were affected by this bug.

As a final aside, I think I'll rename 'loose' to 'permissive'...

jni commented 8 years ago

@elhuhdron switching is easier than you think! Have a look at the "modernize" package to do 99% of the work for you. And read this. Relevant to this context, this bug could easily have been prevented if we'd been using Python 3.5's type hints.

jni commented 8 years ago

@elhuhdron Fixed in master and in 0.3.1! Thank you very much for the report! Please let me know if you encounter any more problems.

Also, if you get any insights into 'strict' vs 'permissive', I'd appreciate a heads-up. I'll need to issue a correction for the PLOS ONE paper, but I want to run some experiments comparing the two modes.

elhuhdron commented 8 years ago

No problem. That was speedy, thanks!

Yeah, I'm testing out on our dataset with different options now. I'll let you know.