Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roadmap for v0.1 #4

Open
JoshChristie opened this issue Aug 4, 2017 · 9 comments
Open

Roadmap for v0.1 #4

JoshChristie opened this issue Aug 4, 2017 · 9 comments

Comments

@JoshChristie
Copy link

Hey,

Thanks for this package!

Was the plan for this package to get registered? What needs to be done to get there?

Thanks,

Josh

@visr
Copy link
Owner

visr commented Aug 4, 2017

I think it's a good idea to register this package. Before I do I'd like to go over the API once more, I may get to this next week. If you'd like to see any changes, let me know.

@c42f, your cjf/las14 branch, shall we integrate it before registering? I'd be for supporting 1.4, though don't have experience with dealing with them yet.

Also, I'm using the laszip branch now to pipe through laszip for laz reading/writing. Do you think this belongs in master? I like the package pure julia right now, or could we just add the functions and not provide laszip ourselves? A pure julia laszip implementation would be cool, but quite a project I think.

@c42f
Copy link
Collaborator

c42f commented Aug 4, 2017

I'm really happy to have this package, but the API, I think, could definitely be refined a bit. Perhaps it's worth considering a simultaneous first release of:

  • Version 0.0.1, with the existing API which after all has been more or less unchanged for a year, and some people have already used for real work, I think.
  • Version 0.1.0 with an improved API, recommended for packages starting to use LasIO after registration?

As to the state of the las14 branch, I'm afraid there's quite a bit of work to get that into shape before we can use it properly. When I started writing the branch, I found myself doing quite a bit of refactoring to generalize the API into a shape which seemed sensible. I didn't finish though before other work intervened.

Thinking back, here's some things which I was thinking about:

  • The raw las header is different in different file versions, so what we have doesn't generalize really nicely.
  • It would be good to have much more library support to help the user create a valid header with one of the default point types. It's really easy to accidentally create and save an invalid header last time I looked.
  • Do users actually care about the raw header on average? It would be nice if they see only a higher level interface by default, with access to the raw header if they know what they're doing.
  • It would be useful to be able to load only the file header in a convenient way, without the points.
  • Returning a tuple of (header,points) from load seems a bit... disjointed? I'm not sure what to do about that though.
  • It might be useful to have low level raw LasPoint types, without the fixed point encoding for the colors. I've seen people use the color channels for other things, and treat them as integers (alas), so sometimes easy access to the raw integer values is useful. There's reinterpret for this.
  • Conversely, the point coordinates don't mean anything without the header. We could try for some high level point types using an "affine" fixed point number type for the x,y,z coordinates which would free the points from their associated file header by storing the scaling and offset in the type. This would still be efficient for large arrays of such points, and it would be permissable to reinterpret an array of low level raw points into such a high level point type. An alternative might be a special LasPointArray <: AbstractArray which could contain the scale and offset, and convert raw las points to las points where the position coordinates are in Float64, on the fly.
  • It would be nice to be conveniently able to use LasIO.load(), independently of FileIO. Loading a las file produces las-specific data structures, so I don't think the FileIO abstraction gives a lot of value here. Not that I mind depending on it, but it might be nice to be able to just use LasIO.load() directly with a path.
  • Streaming and/or mmap. Sometimes I just have a lot of points and it would be nice to stream them to the output, or stream / mmap them for input. Certainly not required for a first release, though having this in mind may affect some of the design decisions.

To some extent these are cosmetic things. API-wise, I guess there's two themes:

  1. Higher level access to the header, both to generalize between las versions, and to allow the user to construct a header in a more robust way, which doesn't require knowing the details of the las spec for each las version.
  2. Have an abstraction where you can treat the array of las points independently from the las header, and still extract meaningful coordinates.

@visr
Copy link
Owner

visr commented Aug 18, 2017

Thanks a lot Chris for your thoughts. I think pretty much all your points can be converted into checkboxes 😄. I'll try to get v0.0.1 registered in the short term. What do you think about merging the laszip branch for v0.0.1 as well? Any additional thoughts about that?

It might be useful to have low level raw LasPoint types, without the fixed point encoding for the colors. I've seen people use the color channels for other things, and treat them as integers (alas), so sometimes easy access to the raw integer values is useful.

I'm not sure about this, the binary representation currently directly corresponds to the spec, that's pretty raw already. The red::N0f16 is created by reinterpret(N0f16, read(io, UInt16)), if people want to use these fields differently they can just reinterpret it back right? Or we could provide constructors that accept UInt16.

@c42f
Copy link
Collaborator

c42f commented Aug 18, 2017

What do you think about merging the laszip branch for v0.0.1 as well

Am I correct in thinking you're just piping through an external laszip binary for this, and there's no additional hard dependencies? In that case, it sounds perfectly reasonable and very useful to merge this. My only reason to suggest 0.0.1 was for backward compatibility with the current somewhat stable API, if we're going to do a big API rethink.

I'm not sure about this, the binary representation currently directly corresponds to the spec

Yes, it's certainly a good thing to conform to the spec by default. To be clear, I was suggesting having both a low level and a high level point type for each point format, with people only seeing the high level by default. But I think you're right that we should just conform to the spec for now, and let people use reinterpret if they want to do something nasty.

(I have observed various las files in the wild which don't really conform to the suggested encoding (for example, treating 255 as the maximum value, as permitted by the 1.1 spec which doesn't have an official stance IIRC). I've also observed people abusing las color fields for other data. It's unfortunate, but these are real files which we sometimes have to deal with!)

@visr
Copy link
Owner

visr commented Aug 18, 2017

there's no additional hard dependencies?
Correct. I'll polish it up and submit a PR. It doesn't break the master API, only extends it to LAZ.

To have both high level and low level interfaces sounds right to me, I indeed don't want to force users to learn the spec.

@c42f
Copy link
Collaborator

c42f commented Aug 18, 2017

Oh by the way, thanks for taking all that in. It ended up being quite a wall of text!

@visr
Copy link
Owner

visr commented Aug 21, 2017

Here is the PR for v0.0.1, with the existing API: JuliaLang/METADATA.jl#10831

@c42f
Copy link
Collaborator

c42f commented Aug 22, 2017

Nice, thanks a lot :-)

@visr
Copy link
Owner

visr commented Aug 23, 2017

It's merged now. I'll leave this issue open as a roadmap for v0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants