search

Deltares / PointCloudRasterizers.jl

Process airborne laser scans into raster images
MIT License
12 stars 3 forks source link

add filter using a raster method, remove flipud! from reduce #6

Closed Crghilardi closed 4 years ago

Crghilardi commented 4 years ago

Closes #5

This PR add a Base.filter!(index::PointCloudRasterizers.PointCloudIndex, raster::GeoArray, condition=nothing) method. I also removed the flipud! which was preventing this from working correctly. I also updated the README and tests.

visr commented 4 years ago

Nice, thanks for adding this method!

Don't we get an upside down reduced raster now on reduce without flipud? I was thinking that we would need to modify the index itself, such that we don't need flipud anymore.

Also, I know that the existing tests only just run the code and not actually test anything, but it would be great to move away from that. Even just a simple count or mean of the reduced results can prevent regressions in the future.

Crghilardi commented 4 years ago

I don't quite understand what the flipud! was doing in the first place, so I don't know if I am comfortable changing the indexing myself.

I did notice the testset was a copy paste from the README but wasn't sure how important that was. I can do some work on that and wrap everything in testsets. Would you want the test changes in this branch or a new one?

Gelukkig Nieuwjaar

evetion commented 4 years ago

flipud! reverses the GeoArray in the y direction and flips the y step in the AffineMap as well. If you would open both .tif files in QGIS, you would see no difference. However, software packages often have a preference for y coordinates increasing (Julia/Python), or decreasing (GDAL). flipud can help with that.

Crghilardi commented 4 years ago

I added a commit that starts to restructure the tests. It is failing on CI but julia .test/runtests.jlpasses for me locally? EDIT: I had some other version in /dev that was active, a clean install fixed the issue.

I can try to do the index work if you can describe exactly what needs to be done. I don't understand the reduce function clearly to make big changes.

visr commented 4 years ago

I had a chat about this with @evetion but he basically said since the y axis direction varies between different packages, it's hard to 'get it right' in the index. So let's leave the index as it is. We can flip it as needed when we are forced to, for instance when saving it with GDAL.

@evetion could you check if this is ok? Or should we put back the flipud that was removed here, and then also do it at the beginning of the new method?

evetion commented 4 years ago

The flipud! should be removed from this repository. I think the positive x, y flows naturally from coding it and will work in all dimensions. Suddenly flipping only the y seems weird. Both GeoStats and Plots need positive axes, it's mostly GDAL, when finally serialized to tiff, complains about it.

Feel free to rework things into @testsets in this branch.

Crghilardi commented 4 years ago

OK, I addressed your comments in the latest commit. I re-ordered the README so filtering comes after raster is declared and cleaned up spacing on various comments. flipud! remains fully removed.

I originally wrote the tests in @testsets but ran into scoping issues. I can revisit tests at a later time, I would like to get this functionality merged so I can finish writing my blogpost.