Radon transform #549

Aravind-Suresh · 2016-02-13T10:19:27Z

Basic version of radon transform implemented.

cbalint13 · 2016-02-13T10:34:39Z

modules/README.md

 19. **opencv_xfeatures2d**: Extra 2D Features Framework containing experimental and non-free 2D feature algorithms.

-20. **opencv_ximgproc**: Extended Image Processing: Structured Forests / Domain Transform Filter / Guided Filter / Adaptive Manifold Filter / Joint Bilateral Filter / Superpixels.
+20. **opencv_ximgproc**: Extended Image Processing: Structured Forests / Domain Transform Filter / Guided Filter / Adaptive Manifold Filter / Joint Bilateral Filter / Superpixels / Radon transform.


Radon transform is within "Domain Transform" category, not sure it must be mentioned as separate.

There is also the Hough Transform present here in ximgproc (Hough is spacial case of Radon).

What should I do then? The outputs of hough and radon are similar. But the way they are implemented is different. Should I remove "Radon transform" from README.md ?

cbalint13 · 2016-02-13T10:43:16Z

@Aravind-Suresh ,

Some improvements suggestions:

Could use parallel_for() for better performance.
Could support any Mat type CV_XX ?
See the hough transform: https://github.com/Itseez/opencv_contrib/blob/master/modules/ximgproc/src/fast_hough_transform.cpp

cbalint13 · 2016-02-13T10:52:37Z

@Aravind-Suresh ,

Based on fast_hough_transform.cpp I wanted in a similar manner (quadrants, universal Mat type) this implementation: http://openrisc.rdsor.ro/radon . Its based on old work of Peter Sestoft PhD on the very subject, I believe the best one in numerical field of radon / inverse radon computation:. See a discrete radon computation in quadrants taht would fit together nice with the actual fast_hough(): http://openrisc.rdsor.ro/radon/myradon.m

Indeed, to be polite I let this PR to flow and perhaps enhance it another time. Consider my opinion only as advice level, no need to follow them at all.

Aravind-Suresh · 2016-02-13T10:57:30Z

Okay. I will have a look into the links suggested. What should I do now? A fast radon implementation or the existing one should be slightly modified?

cbalint13 · 2016-02-13T11:09:09Z

@Aravind-Suresh ,

Folks including I when trying to PR our favorite algo first document and try see if there is something similar already done in opencv. If there is already some similar work done (in our case hough_transform) we try to improve it or add new functionality (e.g now comes the general radon). Quality of implementation matters, simple testcases wherever is possible or some proofs are expected. Proper citations are mandatory, also comments, insights about formulas, from where code/ideas are coming are welcomed.
I salute your effort but regarding the algo quality I am in doubt (correct me if i am wrong). My time is limited to check, also I am a simple user here.

I believe would be great to try implement the exposed matlab piece or from other trusted and well documented sources you like more.

cbalint13 · 2016-02-13T11:14:32Z

@Aravind-Suresh ,

For reference:

Uploaded Peter Toft theses + codes (excelent work): http://openrisc.rdsor.ro/radon/, original here: http://petertoft.dk/PhD

My ultimate goal (personal) would be to see hough aside with radon and inverse radon using parallelism and also a GPU versions too, but my time is limited.

Aravind-Suresh · 2016-02-13T14:58:08Z

I implemented it after looking into the links. Can you tell me what other changes need to be made?

cbalint13 · 2016-02-13T16:40:07Z

@Aravind-Suresh ,

Few ideas starting with the exposed API of fast Hough: https://github.com/Itseez/opencv_contrib/blob/master/modules/ximgproc/include/opencv2/ximgproc/fast_hough_transform.hpp

Would be nice to align radon to that to make a nice tandem in OpenCV . See their ideas, I believe are well reasoned and well done:

Cite the paperwork from draft where you inspired/studied (e.g. Peter Toft, PhD, Year). mandatory citation should go here: https://github.com/Itseez/opencv_contrib/blob/master/modules/ximgproc/doc/ximgproc.bib and should be called/reflected in your include/.hpp as doxygen syntax. You can add to .hpp & .cpp your name / address too in case people want contact you.
See how could handle universal input Mat CV_XX types (CV_32F, CV_64F, CV_8U ...).
See that user could choose AngleRangeOption when calling the transform (done by you too but not exposed to API).
See HoughOp choosable operator (can stack, average, min or max within rho/theta space), this is very interesting and you could add too if want.
See the HoughPoint2Line(const Point &houghPoint) exposed function in the API. In case of radon (RadonPoint2Line) given any rho/theta from the sinogram space a line can be computed back. The whole meaning of Hough/Radon is the ability to sample a point from tho/theta and compute back the very coresponding line. Going further, eficient inverseRadon would be possible thus recovering whole original image from the sinogram (MRI), but that can be postponed as implementation, its not that easy.

Add some example to proof it works. E.g. take two very noisy lines compute and display the sinogram , find biggest two points in sinogram, compute and draw back the lines over the original noisy image. E.g.: http://cogsys.imm.dtu.dk/staff/ptoft/Radon/Radon.html . If you could example this side by side with actual Hough transform proving that radon sinogram is much more sharper in noisy conditions that example would be cherry of the cherries !

When done, assure builbots are in green state and squash all commits into one (ask here if you have troubles, someone will help).

Please consider these as simple advices. Last word will be of the reviewers/maintainers. I am simple contributor here with some care about quality. It happen to be very interested in this radon transform so i tried to advice you in my best ways. My respect goes to You so I let you choose your final ways.

Aravind-Suresh · 2016-02-13T16:43:47Z

Haha, thanks a lot. Will try to implement the best I can.

cbalint13 · 2016-02-14T01:26:27Z

@Aravind-Suresh ,

I hope s/Haha//g vector can be numerically cancel out, I believe is not more than a juvenile observation.
Cite Mr. Peter Toft if decided to inspire / use his code !
Would be nice to proof by some of your examples, perhaps additional test / perf folders.

People spend lot of time documenting & bugfixing, much more spent time then just adding new features.

Looking forward for the best of Radon [1] one, looking forward for best it can be done.

[1] https://en.wikipedia.org/wiki/Radon_transform

Aravind-Suresh · 2016-02-14T04:26:39Z

Okay. I will add new features ( angle range option and operations ) and documentation soon.

Aravind-Suresh · 2016-02-14T07:42:13Z

How should I add support for any type of input? I am thinking of normalizing the input to [0, 1] with CV_32F or CV_64F and go ahead. Is that right? Or is any better alternative available?

cbalint13 · 2016-02-14T09:15:06Z

@Aravind-Suresh ,

I think no need to normalize. Assume the result space is always fixed to CV_32F (thus no overflow will occur with ease). Just document/mention this fixed asumption in API. But input dataset can be e.g CV_8U, CV_23S whatever user wants. I believe sinogram can overflow only if input data is extremely large in size but then computation is intractable too in time. Simple trick like switch (input-img.type() ) would do it e.g: https://github.com/Itseez/opencv_contrib/blob/master/modules/ximgproc/src/fast_hough_transform.cpp#L253
I've used P. Toft's routines even with 32k x 32k large images and never overflowed, not even in uin16 target space. In ideal case of NxN sized raster, longest line for which each pixel generate/accumulate peak value in sinogram would be if wold take the diagonal sqrt(NxN + NxN) in case of 32k x 32k image is ~45254.8339959 value, so is far from overflow. The calculus is empiric not very exact.
If want test out you can add anywhere (temporary) something like feenableexcept( FE_DIVBYZERO | FE_INVALID | FE_OVERFLOW | FE_UNDERFLOW ) at compile time to see. Will trigger segfaults if over/underflows occur.
I realize that in case of Hough they create the same type as input for output: https://github.com/Itseez/opencv_contrib/blob/master/modules/ximgproc/src/fast_hough_transform.cpp#L391 Now that not sound too good. I will check it later maybe in hough space not overflow but i doubt in the case of CV_8U target even with image size of 1k * 1k.

Aravind-Suresh · 2016-02-14T10:41:02Z

Okay. Thanks. I will try to do modify the code so as to process any type of input, keeping output depth fixed.

Aravind-Suresh · 2016-02-15T07:48:47Z

Is the code fine? If it is, I will complete documentation and example code, and make a final commit. Thanks.

cbalint13 · 2016-02-15T09:52:51Z

@Aravind-Suresh ,

Whow ! Very elegant work, Thank You very much for taking care !

Few minor comments, little inconsistency:

Optional operation should default to RT_SUM (accumulation mode) not -1 (which is even undefined state according to enum RadonOp). People will be confused that radon does nothing. Also please fill some integer values to enum RadonOp like for enum ARFlags
Optional AngleRange now default to -1 (nonexistent value according to enum ARFlags), I believe you wanted to set to 32 (since AR_180 = 32).

After you finish and all is green merge all individual commits into single one for the reviewers (by git rebase). Also notice that review can take up to several month depending on relase cycle/upstream time schedule so be patient.

Aravind-Suresh · 2016-02-15T10:03:24Z

I wanted to put 63 for AngleRange as it would include all the angle between 1 and 180 degrees. Will do the same.

Also, for the operation should I give a default value, as people may not need to perform an operation. They can just get the Radon transform and analyse it. Or should I create an enum value RT_NONE which I will put as default ?

cbalint13 · 2016-02-15T11:15:20Z

@Aravind-Suresh ,

RT_NONE make no sense. All OpenCV functions does something at all, there is no such "do nothing". I believe the RT_SUM is the default radon behaviour as original formula, all other are special cases of radon space formation.

Aravind-Suresh · 2016-02-15T11:24:55Z

Okay. I will add RT_SUM as the default value.

Aravind-Suresh · 2016-02-15T14:24:41Z

I am getting a merge conflict error with the buildbot. Can somebody help me fix it?

mshabunin · 2016-02-15T14:58:44Z

Make sure you have a 3-way visual merge tool installed and configured in your Git config.

Run git fetch upstream master (assuming upstream remote points to https://github.com/Itseez/opencv_contrib.git) then git rebase upstream master.
If process stops asking you to fix merge conflicts - run git mergetool to fix them (possibly manually).
When conflicts are solved - run git rebase --continue.
Repeat steps 2-3 if requested.
After rebase push to your remote: git push -f origin radon-transform (assuming origin points to your opencv_contrib fork)

Aravind-Suresh · 2016-02-15T15:13:20Z

Ohh okay. Thanks. Merged and pushed.

Aravind-Suresh · 2016-02-16T07:35:12Z

I completed the implementation and it has passed the required builds. Please review it and merge it. Suggestions are welcome :) .

dogramacigokhan · 2016-02-26T13:14:10Z

@Aravind-Suresh

I subtracted the min angle values from the theta values as a workaround and now it works properly.

Thanks!

Aravind-Suresh · 2016-02-27T07:48:20Z

@dogramacigokhan I made the change which you pointed out. Thanks :)

Aravind-Suresh · 2016-02-27T11:06:36Z

I am getting an error like this:

fatal: Unable to create '/build-3/precommit-contrib_linux64/opencv_contrib/.git/refs/heads/master.lock': File exists.

If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.

Can anyone tell me how to fix it?

Dikay900 · 2016-02-27T12:36:11Z

look if there is another git process accessing the repo.
If not then simply remove the lock file and try again.
This is happening if a git command could not end properly or was closed while still running in background.

dogramacigokhan · 2016-03-01T06:59:20Z

@cbalint13

Is it necessary to squash commits into one if someone else pushes commits to the current pull request, too? I'm planning to make some major changes on code. Should I wait for this pull request to be merged or should I create pull request to current one?

alalek · 2016-03-01T08:09:21Z

someone else pushes commits to the current pull request

GitHub forbids this by default.

Anyway you can start experiments in new own PR based on commit from this patch (2+ commits in total).

cbalint13 · 2016-03-01T08:17:33Z

@dogramacigokhan ,

PR should be in a compact state, so commits needs to be squashed when you consider finished. You can change whatever you want even now, there is no need to wait to merge and then to commit changes again. Review time can take up to six month based on opencv_contrib release cycle so be patient for the final review & merge.
There is another method (force push) to compact your changes at any time, i found this way much more easy and more confortable than git rebase. But be aware it overrides all your logs and changes (but this is the scope of squashing, except the loss of logs). But it guarantees that you will end up with single compact PR and also keeps the track with any upstream changes/conflicts (if also have case of conflicts with upstream):

01) wget https://github.com/Itseez/opencv_contrib/pull/549.patch #(safe copy)
02) git clone https://github.com/Aravind-Suresh/opencv_contrib
03) cd opencv_contrib
04) git pull https://github.com/Itseez/opencv_contrib.git
05) git push #(optional, only if there where changes at pt. 4)
06) git checkout radon-transform
07) git reset --hard origin/master
08) git apply ../549.patch --reject #(restore the copy, make sure with no errors)
09) #(can do any changes you would like to the code)
10) git commit -a -m "Implement Radon Transform."
11) git push --force

cbalint13 · 2016-03-01T13:56:35Z

@Aravind-Suresh , @dogramacigokhan

I think it is possible to have multiple 'commiters' on same forked branch thus changing.controlling the same PR (in this case @Aravind-Suresh 's branch called radon-transform). All you need is to obtain 'colaborator' status from @Aravind-Suresh (see github profile settings for that fork, or global settings, just google for that).

But, even in such scenario whith single/multiple commiters someone (one of you two who have access to forked branch) need to squash all the commits, thus by squashing i think only @Aravind-Suresh 's name will be displayed as author).

Aravind-Suresh · 2016-03-01T14:06:01Z

Okay. @dogramacigokhan I have added you as a collaborator. Start working on radon transform and commit your works. As @cbalint13 said, we will squash them later.

tomsfernandez · 2017-12-16T16:19:48Z

Hi, was this request finally merged?

Aravind-Suresh · 2017-12-17T07:31:29Z

Hi @tomasfernandezm,

No. It hasn't been merged yet.

terfendail

Could you please also rebase your changes to latest master to handle merge conflicts

terfendail · 2018-04-27T14:33:26Z