Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Local Thresholding Algorithm #10

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

New Local Thresholding Algorithm #10

wants to merge 42 commits into from

Conversation

nguy8tri
Copy link

Horizontal Thresholds to replace the old cog algorithm. (A subdivision of 1 defaults back to old algorithm)

@markasoftware
Copy link
Member

First TODOs, as we talked about today:

  • Convert all newlines to Unix newlines. There is a command line tool dos2unix which may help.
  • Remove all cairo files.

You can do this by simply adding a new commit fixing the problems.

If you're feeling like doing some advanced Git work, a fancy way to fix these issues is by doing an "interactive rebase", which lets you modify individual commits from history.

@markasoftware
Copy link
Member

Thanks for removing cairo! However, I notice that in your commit changing to LF, only one file was modified. All the other files need to be fixed too. This may be as simple as running dos2unix src/*.cpp in the terminal.

I also now notice that you committed the lost-master directory back in November. This should be deleted.

@markasoftware
Copy link
Member

I see that all the extra files are now removed! Could you also fix the newlines using dos2unix or a similar tool? If you go to the "files changed" tab here on Github, you can see that all the files are still modified.

@markasoftware markasoftware changed the base branch from master to 2DGF February 25, 2022 02:08
@markasoftware markasoftware changed the base branch from 2DGF to master February 25, 2022 02:08
@markasoftware markasoftware changed the base branch from master to ML-LOST February 25, 2022 02:23
@markasoftware markasoftware changed the base branch from ML-LOST to master February 25, 2022 02:23
Copy link
Member

@markasoftware markasoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've suggested some changes.

One thing that would really clean up your code is to modify LocalBasicThresholding so that instead of explicitly calculating the beginning and end of each subdivision, it uses functions eg BeginningOfSubdivision(width, height, i), EndOfSubdivision(width, height, i), then you just loop for j between the beginning and end returned by the functions. This way you can centralize all the fiddly math related to leftovers, etc.

src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
test/main.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
@nguy8tri
Copy link
Author

nguy8tri commented Mar 3, 2022

Hi Mark, I just finished implementing all the changes, thanks for pointing out the faults for me! However, I can't test it since I don't have any Cairo files or my kvector.dat file anymore.

src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Show resolved Hide resolved
test/centroid.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
test/centroid.cpp Outdated Show resolved Hide resolved
src/centroiders.cpp Outdated Show resolved Hide resolved
test/centroid.cpp Outdated Show resolved Hide resolved
@markasoftware
Copy link
Member

It's looking pretty good, I need to actually test it myself but after that I think it's just about done!

@nguy8tri
Copy link
Author

Thanks. I just added one more change, but I think it should be good now!

@nguy8tri
Copy link
Author

nguy8tri commented Sep 7, 2022

There is an issue that is a bit confusing. In this rendition of COG and IWCOG, it rounds the threshold, wheras in the original it floors it, yielding different results. This was a change you actually requested, so its up to you whether you'd like to let it round or floor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants