-
Notifications
You must be signed in to change notification settings - Fork 544
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
Feature/geometry data points filter for master #374
Feature/geometry data points filter for master #374
Conversation
Can one of the admins verify this patch? |
ok to test |
but containing just eigen-values. Also, Jenkins suggested fabs() instead of abs().
Jenkins has some error related to be unable to read some output log file (java exception in the end of the console output). Seems to me more like Jenkins configuration problem than the actual build and test problem... |
that's a recurring problem and why we want to transfer ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kubelvla,
Your contribution is very nice. I reviewed your pull request and left some comments.
Considering that this filter is a new feature, would it be possible to introduce a small set of unit tests for basic cases?
@@ -0,0 +1,170 @@ | |||
// kate: replace-tabs off; indent-width 4; indent-mode normal | |||
// vim: ts=4:sw=4:noexpandtab |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @YoshuaNava,
thanks a lot for the review! I based the source files on another filter, which had this type of header. Now I've double-checked all the other files and they are all like this. I guess it is a practical thing to have it there. I'll keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
||
// Eigenvalues | ||
#include "Eigen/QR" | ||
#include "Eigen/Eigenvalues" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Libpointmatcher have any policy regarding internal/external includes?
For example, https://github.com/ethz-asl/segmap/wiki/Contributing-to-SegMap represents external include paths between <>, and internal ones with ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually forgot to remove these includes, they are from the original file I took as a model and they are probably not needed at all... Nevertheless, I haven't heard about such internal convention. @pomerlef do you have a strong opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no policy was close to most of our policies, so no.
If it help readability, it can be added.
François Pomerleau and Stephane Magnenat, ASL, ETHZ, Switzerland | ||
You can contact the authors at <f dot pomerleau at gmail dot com> and | ||
<stephane at magnenat dot net> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest asking the author if you can also add yourself here, not only to ack your contribution, but as a way of informing people of who wrote the file, and maybe providing contact information and a mention of the papers where this method is described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually about to put myself there (in one commit, I had there TODO), but then all the files, even written by our students, have this copyright header. So I was not sure if by changing it, I would not have messed some legal stuff (there is the 2010--2018, so if I put myself there, do I have to somehow extend the copyright or not...). @pomerlef How to add a note about the author? Below the copyright text?
|
||
inline static const std::string description() | ||
{ | ||
return "This filter computes the level of ‘unstructureness’, ‘structureness’ and ’sphericality’ for each point based on the required eigen values.\n\n" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll rename the filter to Sphericality, it also suggests the required 3D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added more details on what that actually does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! Thank you.
const bool keepStructureness; | ||
|
||
GeometryDataPointsFilter(const Parameters& params = Parameters()); | ||
virtual ~GeometryDataPointsFilter() {}; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that both ways are clear to understand and the empty-body style is common to this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
ok to test |
@kubelvla Would it be possible to add the unit tests for this new filter? Additionally, will you add it to the documentation within the scope of this PR?
|
For the unit tests, I'll think about what should actually be tested and then ask one of our students to help me. The bibliography will be updated as soon as the paper is out, now waiting for review. |
Hi @kubelvla, I understand. I would propose to merge as soon as possible if you and the author approves. Hence, can we open an issue about creating tests and documentation for this filter? The reason to merge as soon as possible is because I want to propose a clang format set of rules and if approved apply them to the repo. In my experience that takes time, thus the earliest we start, the better. |
@YoshuaNava I'll open the issue with the biblio and unit test for myself. |
@kubelvla Looks good to me. |
@kubelvla What is the status of this PR? Do you have further implementations planned? |
@YoshuaNava Under the condition I made no mistake when implementing the filter, there should be no changes to this filter. |
@kubelvla Is this PR ready for merging? |
If there is no objection, I'll merge it tomorrow. |
@pomerlef No objection from my side. |
This filter reproduces our heuristic intended for estimating radio signal attenuation through point clouds (GPS and UGF signals).