Feature/geometry data points filter for master #374

kubelvla · 2020-04-22T21:40:33Z

This filter reproduces our heuristic intended for estimating radio signal attenuation through point clouds (GPS and UGF signals).

…le code.

ethzasl-jenkins · 2020-04-22T21:40:35Z

Can one of the admins verify this patch?

pomerlef · 2020-04-22T21:43:57Z

ok to test

but containing just eigen-values. Also, Jenkins suggested fabs() instead of abs().

kubelvla · 2020-04-23T00:11:28Z

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...

pomerlef · 2020-04-23T00:59:44Z

that's a recurring problem and why we want to transfer ownership

YoshuaNava

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?

kubelvla · 2020-04-23T21:05:41Z

pointmatcher/DataPointsFilters/Geometry.cpp

@@ -0,0 +1,170 @@
+// kate: replace-tabs off; indent-width 4; indent-mode normal
+// vim: ts=4:sw=4:noexpandtab


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.

YoshuaNava · 2020-04-23T19:24:39Z

pointmatcher/DataPointsFilters/Geometry.cpp

+
+// Eigenvalues
+#include "Eigen/QR"
+#include "Eigen/Eigenvalues"


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 ""

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?

no policy was close to most of our policies, so no.

If it help readability, it can be added.

pointmatcher/DataPointsFilters/Geometry.cpp

YoshuaNava · 2020-04-23T19:51:33Z

pointmatcher/DataPointsFilters/Geometry.h

+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>
+


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.

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?

pointmatcher/DataPointsFilters/Geometry.h

kubelvla · 2020-04-23T23:44:09Z

pointmatcher/DataPointsFilters/Geometry.h

+
+	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"


I think I'll rename the filter to Sphericality, it also suggests the required 3D

Also added more details on what that actually does.

Cool! Thank you.

kubelvla · 2020-04-24T19:03:58Z

pointmatcher/DataPointsFilters/Geometry.h

+	const bool keepStructureness;
+
+    GeometryDataPointsFilter(const Parameters& params = Parameters());
+	virtual ~GeometryDataPointsFilter() {};


I think that both ways are clear to understand and the empty-body style is common to this library.

pointmatcher/DataPointsFiltersImpl.h

pomerlef · 2020-04-23T20:17:46Z

ok to test

YoshuaNava · 2020-04-23T22:37:41Z

@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?

Tutorials.
Bibliography.cpp https://github.com/ethz-asl/libpointmatcher/blob/master/pointmatcher/Bibliography.cpp

…r name.

kubelvla · 2020-04-24T19:19:24Z

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.

YoshuaNava · 2020-04-24T19:45:44Z

Hi @kubelvla,
Thank you for addressing the comments.

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.

pointmatcher/DataPointsFilters/Sphericality.cpp

kubelvla · 2020-04-24T21:03:05Z

@YoshuaNava I'll open the issue with the biblio and unit test for myself.

YoshuaNava · 2020-04-29T15:36:23Z

@kubelvla Looks good to me.

YoshuaNava · 2020-05-15T20:25:20Z

@kubelvla What is the status of this PR? Do you have further implementations planned?

kubelvla · 2020-05-18T14:17:39Z

@YoshuaNava Under the condition I made no mistake when implementing the filter, there should be no changes to this filter.

YoshuaNava · 2020-06-08T18:22:42Z

@kubelvla Is this PR ready for merging?

…ster

pomerlef · 2020-07-22T19:39:02Z

If there is no objection, I'll merge it tomorrow.

YoshuaNava · 2020-07-22T20:30:57Z

@pomerlef No objection from my side.

kubelvla added 4 commits April 22, 2020 15:32

Filter files copied from another filter to create an example compilab…

5c69494

…le code.

Filter implemented.

dea74e0

Fixing a nasty bug causing a segfault and some cleanup.

fd14046

Final cleanup.

25b787d

Renamed a variable which was incorrectly suggesting an eigen-vector,

a9b90ef

but containing just eigen-values. Also, Jenkins suggested fabs() instead of abs().

YoshuaNava reviewed Apr 23, 2020

View reviewed changes

kubelvla added 3 commits April 23, 2020 23:25

Cleaning up for a better pull request.

44a75c4

Addressing comments from YoshuaNava and also giving the class a bette…

a2bd95a

…r name.

Changing behavior for the extreme case when two eigenvalues are zero.

3400797

Renaming the files accordingly to the renamed class.

9f4127b

YoshuaNava approved these changes Apr 24, 2020

View reviewed changes

pointmatcher/DataPointsFilters/Sphericality.cpp Show resolved Hide resolved

YoshuaNava approved these changes Apr 29, 2020

View reviewed changes

Merge branch 'master' into feature/geometry_data_points_filter_for_ma…

c11b9ab

…ster

pomerlef merged commit 956cb8f into norlab-ulaval:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/geometry data points filter for master #374

Feature/geometry data points filter for master #374

kubelvla commented Apr 22, 2020

ethzasl-jenkins commented Apr 22, 2020

pomerlef commented Apr 22, 2020

kubelvla commented Apr 23, 2020

pomerlef commented Apr 23, 2020

YoshuaNava left a comment •

edited

This comment was marked as resolved.

kubelvla Apr 23, 2020

YoshuaNava Apr 29, 2020

YoshuaNava Apr 23, 2020

kubelvla Apr 23, 2020

pomerlef Apr 23, 2020

YoshuaNava Apr 23, 2020

kubelvla Apr 23, 2020 •

edited

This comment was marked as resolved.

kubelvla Apr 23, 2020

kubelvla Apr 24, 2020

YoshuaNava Apr 29, 2020

This comment was marked as resolved.

kubelvla Apr 24, 2020

YoshuaNava Apr 29, 2020

pomerlef commented Apr 23, 2020

YoshuaNava commented Apr 23, 2020 •

edited

kubelvla commented Apr 24, 2020

YoshuaNava commented Apr 24, 2020 •

edited

kubelvla commented Apr 24, 2020

YoshuaNava commented Apr 29, 2020

YoshuaNava commented May 15, 2020

kubelvla commented May 18, 2020

YoshuaNava commented Jun 8, 2020

pomerlef commented Jul 22, 2020

YoshuaNava commented Jul 22, 2020

		@@ -0,0 +1,170 @@
		// kate: replace-tabs off; indent-width 4; indent-mode normal
		// vim: ts=4:sw=4:noexpandtab

Feature/geometry data points filter for master #374

Feature/geometry data points filter for master #374

Conversation

kubelvla commented Apr 22, 2020

ethzasl-jenkins commented Apr 22, 2020

pomerlef commented Apr 22, 2020

kubelvla commented Apr 23, 2020

pomerlef commented Apr 23, 2020

YoshuaNava left a comment • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubelvla Apr 23, 2020 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomerlef commented Apr 23, 2020

YoshuaNava commented Apr 23, 2020 • edited

kubelvla commented Apr 24, 2020

YoshuaNava commented Apr 24, 2020 • edited

kubelvla commented Apr 24, 2020

YoshuaNava commented Apr 29, 2020

YoshuaNava commented May 15, 2020

kubelvla commented May 18, 2020

YoshuaNava commented Jun 8, 2020

pomerlef commented Jul 22, 2020

YoshuaNava commented Jul 22, 2020

YoshuaNava left a comment •

edited

kubelvla Apr 23, 2020 •

edited

YoshuaNava commented Apr 23, 2020 •

edited

YoshuaNava commented Apr 24, 2020 •

edited