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

[2d] Remove variable assignments that are never used #3857

Merged

Conversation

diivm
Copy link
Contributor

@diivm diivm commented Apr 3, 2020

2d/include/pcl/2d/impl/kernel.hpp Outdated Show resolved Hide resolved
Co-Authored-By: SunBlack <SunBlack@users.noreply.github.com>
@SunBlack
Copy link
Contributor

SunBlack commented Apr 3, 2020

As far as I can see in hessianBlob:

  • Scope of local_max could reduced to const float local_max = cornerness[k][i][j];
  • Value of scale_max is not used anywhere, so it can be reduced and if (cornerness[k][i][j] > scale_max) can be changed to if (cornerness[k][i][j] > std::numeric_limits<float>::min())

@SunBlack
Copy link
Contributor

SunBlack commented Apr 3, 2020

Can it be that hessianBlob is broken?
Shouldn't this

output[i][i] = start_scale * pow(scaling_factor, k);

be output[i][j] instead of output[i][i]

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

LGTM apart from the comment

2d/include/pcl/2d/impl/kernel.hpp Show resolved Hide resolved
@kunaltyagi kunaltyagi added module: 2d needs: feedback Specify why not closed/merged yet needs: more work Specify why not closed/merged yet labels Apr 3, 2020
@diivm
Copy link
Contributor Author

diivm commented Apr 4, 2020

@SunBlack

  • Scope of local_max could reduced to const float local_max = cornerness[k][i][j];

Agree.

  • Value of scale_max is not used anywhere, so it can be reduced and if (cornerness[k][i][j] > scale_max) can be changed to if (cornerness[k][i][j] > std::numeric_limits<float>::min())

Why should we check every time with std::numeric_limits<float>::min()?
It stores the previous value of cornerness[k][i][j] for comparison with the updated value after the loop rerun. It's value is not reset to std::numeric_limits<float>::min() in the loop, that initialisation is outside the loop (the 'k' loop).

Correct me if I'm wrong.

@SunBlack
Copy link
Contributor

SunBlack commented Apr 4, 2020

You are right, I missed the loop for (int k = 0; k < num_scales; k++) { nevertheless: The scope of scale_max can be reduced ;-)

@diivm diivm requested a review from kunaltyagi April 8, 2020 11:10
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Apr 8, 2020
@SergioRAgostinho SergioRAgostinho added the changelog: fix Meta-information for changelog generation label Apr 8, 2020
@taketwo taketwo merged commit b21f2c5 into PointCloudLibrary:master Apr 13, 2020
@taketwo
Copy link
Member

taketwo commented Apr 13, 2020

🤦‍♂️ should have squashed

@SunBlack
Copy link
Contributor

Force push to master :-P

@taketwo
Copy link
Member

taketwo commented Apr 13, 2020

I realized I made a mistake the same moment I pressed "merge", so I guess it would have been kinda okayish to force push immediately after... But our master branch is protected anyway 🤷‍♂️

@SunBlack
Copy link
Contributor

Isn't it just a setting, which you can turn off shortly?

@taketwo taketwo changed the title [2d] variable assigned a value which is never used [2d] Remove variable assignments that are never used May 10, 2020
@diivm diivm deleted the fix/2d_remove_unwanted_variables branch May 15, 2020 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: 2d needs: code review Specify why not closed/merged yet needs: feedback Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants