terraref / computing-pipeline

Pipeline to Extract Plant Phenotypes from Reference Data
BSD 3-Clause "New" or "Revised" License
23 stars 13 forks source link

Combine las files during ply2las #237

Closed dlebauer closed 7 years ago

dlebauer commented 7 years ago

Is there a reason to keep the east and west las files separate, or can these be merged into a single file?

Completion Criteria

solmazhajmohammadi commented 7 years ago

I dont see a reason to keep east and west files, as long as we have ply files we should be OK. The routine to combine the pair is listed in issue https://github.com/terraref/computing-pipeline/issues/98 , under suggestion section.

ZongyangLi commented 7 years ago

Totally agree! I would prefer a single file for each scan.

Is there anywhere i can find such an example of combined las file?

ghost commented 7 years ago

@max-zilla plz help

dlebauer commented 7 years ago

Or @ZongyangLi could you update the extractor? I know that there is a lasmerge function in lastools.

solmazhajmohammadi commented 7 years ago

Following command will do the merge docker run -v /Path/ToThe/File/data:/data -t pdal/pdal pdal merge /data/InputEast.las /data/InputWest.las /data/output/MergedOutput.las

max-zilla commented 7 years ago

I'll add this command and redeploy this afternoon.

max-zilla commented 7 years ago

This will take a slight tweak to extractor logic, but I don't think it's a big deal:

changes:

this is all trivial boilerplate code but I just wanted to make the change known.

max-zilla commented 7 years ago

https://github.com/terraref/extractors-3dscanner/pull/2/files FYI.

dlebauer commented 7 years ago

alternatively, would it make sense to have a separate extractor that combines two las files and deletes the originals?

On Wed, Jan 25, 2017 at 12:08 PM Max Burnette notifications@github.com wrote:

https://github.com/terraref/extractors-3dscanner/pull/2/files FYI.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/terraref/computing-pipeline/issues/237#issuecomment-275185742, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcX5yuI7WcLRrDeOuEq5LF0qvAeUF4wks5rV4-HgaJpZM4LplFu .

max-zilla commented 7 years ago

@dlebauer I considered that, but my argument would be that is unnecessary extra complexity to write a whole separate extractor. We don't currently have PLY->LAS conversions we want to do WITHOUT merge, and it didn't seem like having a _east LAS file while the _west LAS file (& merge) are still pending would be very valuable alone?

Either way we arrive at the same destination, but this way we handle both steps at once and we don't have to worry about deleting files from Roger/Clowder (which is not something that really fits into other extractor workflows anyway - we've been very wary of deletion)

FWIW I've already added the tweaks to this extractor to accomplish this and am going to test soon. if you'd rather have 2 separate stages we can still do so in the near future.

dlebauer commented 7 years ago

Up to you. I wasn't sure if that would make it easier to do post processing rather than re-generating all of the files. e.g. when we update the in-file metadata we won't want to re-compute (especially if/when we do so for hyperspectral data).

ghost commented 7 years ago

Max will keep the East and West files AND create a merged file. What outputs are needed?

max-zilla commented 7 years ago

@ZongyangLi @solmazhajmohammadi 3 options:

we start with 2 east/west PLY files

option 1 - 2 PLY, 1 LAS (this is current version) keep 2 east/west PLY files create 1 merged LAS file

option 2 - 2 PLY, 3 LAS keep 2 east/west PLY files create east/west LAS files create 1 merged LAS file

option 3 - 3 PLY, 1 LAS keep 2 east/west PLY files create 1 merged PLY file create 1 merged LAS file

@ZongyangLi is option 1 good for you, or do you need 2 or 3?

ZongyangLi commented 7 years ago

@max-zilla option 1 works for me. It's just @dlebauer comment that 'deletes the originals', and I think it will be better to keep 2 existed ply files. Sorry for the misunderstanding, @dlebauer was saying deletes 2 las files.

solmazhajmohammadi commented 7 years ago

Option 1 works for me as well, I merge the ply files in my extractor.

ghost commented 7 years ago

@ZongyangLi and @solmazhajmohammadi can this issue be closed?

dlebauer commented 7 years ago

Last I checked these are still generated separately On Thu, Feb 23, 2017 at 10:42 AM Rachel Shekar notifications@github.com wrote:

@ZongyangLi https://github.com/ZongyangLi and @solmazhajmohammadi https://github.com/solmazhajmohammadi can this issue be closed?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terraref/computing-pipeline/issues/237#issuecomment-282047826, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcX5wabkjFj2FaoDy1O-2M73d0xXyRkks5rfbb-gaJpZM4LplFu .

max-zilla commented 7 years ago

I have added code to accomplish this, but haven't yet back-processed old data to generate merged files. It should occur on new files, however - will close this and include a note to reprocess in #221.