Please fix a bug when writing rowIndex or columnIndex in an e57 file.

[TonnC]

Hello!

I think I found a bug in this fantastic libary. I use it for my job, so I thought: lets contribute here. The issue is as follows: when writing a certain e57 pointcloud we experienced C++ exceptions (E57Exception "ErrorValueOfBounds") bubbling through from C++. I could trace the problem to the following line block:

/pye57/e57.py code around line 379 on main in write_scan_raw:

if "rowIndex" in data and "columnIndex" in data:
    min_row = np.min(data["rowIndex"])
    max_row = np.max(data["rowIndex"])
    min_col = np.min(data["columnIndex"])
    max_col = np.max(data["columnIndex"])
    points_prototype.set("rowIndex", libe57.IntegerNode(self.image_file, 0, min_row, max_row))
    field_names.append("rowIndex")
    points_prototype.set("columnIndex", libe57.IntegerNode(self.image_file, 0, min_col, max_col))
    field_names.append("columnIndex")

The problem is this zero here for rowIndex and below for columnIndex (second parameter in IntegerNode points_prototype.set("rowIndex", libe57.IntegerNode(self.image_file, 0, min_row, max_row))). If e.g. min_row is larger than zero (because our use-case is to filter certain points in the cloud), this code fails with a C++ exception. Because this zero is also tested for min_row and max_row in the C++ code (I debugged this with a sample project). Could we replace this 0 with min_row and min_col below to be safe here? I would open a PR to do it, but I don't have rights to do so I guess. Or please direct me. The solution should be:

    max_col = np.max(data["columnIndex"])
    points_prototype.set("rowIndex", libe57.IntegerNode(self.image_file, min_row, min_row, max_row))
    field_names.append("rowIndex")
    points_prototype.set("columnIndex", libe57.IntegerNode(self.image_file, min_col, min_col, max_col))
    field_names.append("columnIndex")`

Thanks for your help! Cheers.
Christian

[dancergraham]

Hello Christian, thank you for a very clearly written bug report - yes please open a PR for this. If you can show me any way to test it myself, or even better add a test to the current test file, that would be even better.

[TonnC]

I will try to write a PR with test tomorrow in the morning. Thanks, for the fast response! :-)

[TonnC]

Here is the PR with test: #81

[dancergraham]

Thanks I will have a look next week. At first glance it looks good, it would just be nice to use the same pytest fixture as the other tests - I will try to implement that unless you want to do it?

[TonnC]

Hi! I can try to modify the test to use a fixture. No problem. I am originally a C++ developer and new to testing with Python. But I will try. Thanks for the advice! Best regards, Christian

[TonnC]

I updated the PR with using the existing fixture and some minor refactoring. Thanks!

[dancergraham]

I am originally a C++ developer

I would be grateful for any comments and improvements you can make on the cpp wrapping code, if you have the time- I'm very much a python dev and out of my depth with the cpp bits!

[TonnC]

Sure, I can try to help here. Thanks for your support, too. Do you have any special things in mind? I think the first thing I would aim to improve is the pass through of the additional C++ exception infos, like source file and line number of the true error source. They should be all present and only need to get displayed in Python. (I created a small C++ test project to hunt this bug here … to get better debug infos) This might help in the future. But to be clear: I am working in Python since 1.5 years now. I am no expert in writing the C++ bindings yet. But I would like to learn.

[dancergraham]

Do you have any special things in mind? I think the first thing I would aim to improve is the pass through of the additional C++ exception infos, like source file and line number of the true error source

That sounds awesome!

I also tried to update the underlying libe57format library using the git submodules command to advance through the version numbers. I managed some updates but got stuck at one point - I don't remember whether it was due to deprecations or updated dependencies / compiler / c++ version...

[TonnC]

I can try to take a look at this. Let’s see … I don‘t know, how much time I can find to spend on this … after all: christmas is approaching … but, thumbs up. 😊

[dancergraham]

No worries if you have no time, but if you are able to just make a few notes or comments in a new issue then maybe someone can pick it up later. Thanks again for the PR

[TonnC]

Here is the next PR with this C++ exception improvement I had in mind: #84 It did not take so long. :-) What do you think? By the way: is it ok to continue chating here, if it is another issue? How strict are you about correct documentation with issues and PRs? I am used to discussing issues with my reviewer directly in the PR, not in the issue description. But, no worries ... whatever you prefer. :-)

[dancergraham]

close by #81