search

lucasb-eyer / pydensecrf

Python wrapper to Philipp Krähenbühl's dense (fully connected) CRFs with gaussian edge potentials.
MIT License
1.94k stars 414 forks source link

Indexing #20

Closed reynoldscem closed 8 years ago

reynoldscem commented 8 years ago

Due to the special case of '0' indicating no label and being assigned uniform probs, and M being the number of labels not including this I believe the original function was not correct, as the highest label would be out of bounds, and the first label mapped to row 1 of the unary matrix.

The above minor change should fix this, ergo label 1 corresponds to row 1, the number of rows is equal to M, labels go from 1 to M.

(Can show an example if required)

lucasb-eyer commented 8 years ago

You may be right, but I need to look into this more closely, because the examples worked just like in the C++ library last time I tried, so now I wonder how comes.

Also, your solution is not technically correct either, because 0-1 == -1 it will put p_energy to the last class for the pixels having 0 class. But it still works because the next line overwrites those to u_energy.

Could you run the example code on the example images and see if there's a difference? If there's none, can you explain why? I currently don't have much concentration, but can look into it more deeply soon.

lucasb-eyer commented 8 years ago

This really tells me I should write unittests :smile:

reynoldscem commented 8 years ago

Yeah that's fair enough. I will put together a small analysis when I am not in work.

Incidentally, thank you for making this wrapper - it's been v. useful.

lucasb-eyer commented 8 years ago

I have looked into this and written a unit-test. Your solution is indeed correct and the previous example is probably just wrong. I'm merging this and adding tests in a follow-up commit or PR. Thanks!

Also pinging @markusnagel who ported my (wrong) example code to the utils, just in case this change affects his code!

lucasb-eyer commented 8 years ago

Note for clarification: this didn't surface in the examples because M was one too large as it also counted the 0 label. I'll push a fix to the examples later today.

reynoldscem commented 7 years ago

Excellent, good to hear. Apologies for not replying sooner, I am not great with keeping up to date on Github notifications.