gadomski / las-rs

Read and write ASPRS las files, Rust edition.
MIT License
73 stars 32 forks source link

feat: add laz-parallel feature #70

Closed tmontaigu closed 6 months ago

tmontaigu commented 6 months ago

This adds the laz-parallel feature (which enables laz-rs if not already enabled).

When enabled, las-rs will use laz-rs parallel decompressor when reading many points at once (read_n_into, read_next_points) improving the read speed

For example, using the file linked in #64:

The same kind of work can be done for writing LAZ

gadomski commented 6 months ago

@tmontaigu what are the reasons someone would not want to use laz-parallel?

tmontaigu commented 6 months ago

Apart from wanting to avoid compiling unused dependencies I don't see any

gadomski commented 6 months ago

Then I'm inclined to flip the flags — make laz be the parallel version, and add a laz-single-threaded (or something) to be the more verbose but smaller type. I think most folks will want parallel (who doesn't like speed?).

tmontaigu commented 6 months ago

One slight problem with that is that the features laz and laz-single-threaded, would become mutually exclusive features, as in, if one gives both laz and laz-single-threaded feature, that laz would still work in parallel mode.

And mutually exclusive features should generally be avoided (https://doc.rust-lang.org/cargo/reference/features.html?highlight=additive#mutually-exclusive-features)

So we can either:

  1. Ignore this potential thing (and so if somehow due to the depency graph makes it so that both laz and laz-single-threaded are given, parallel mode takes priority)
  2. Only have a laz feature which always enable the parallel mode of laz (and wait for user to come in and say "yes I actually want to avoid compiling the extra dependencies needed for parallelism support)
gadomski commented 6 months ago

Only have a laz feature which always enable the parallel mode of laz (and wait for user to come in and say "yes I actually want to avoid compiling the extra dependencies needed for parallelism support)

I think because it's a extra feature on laz, it makes sense to keep it as a configurable option on las.

I played around with implementing this morning, and I don't love how it looks — I think I'll just keep the status quo (the way things were set up by this PR). Thanks for chatting it through and the contribution @tmontaigu ❤️ .