-
Notifications
You must be signed in to change notification settings - Fork 25
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
improvements and example for linesplitting #228
Conversation
Hi @asinghvi17 and @haakon-e , I am hoping you could give this PR a look while the topic is fresh in our minds. Should have time to iterate upon review next week if PR cannot be merged as is. Thanks for everything |
ohh... this is a good use case. If will be great to have it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this forward! Code looks good. Granted I'm not very familiar with usage of @lift
, so I don't fully understand how/why lift
ing is necessary for this.
I left a few comments and suggestions in the docs that should be equivalent to what you wrote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! I haven't been able to look at this in full detail yet, but had a few questions about return types etc.
src/linesplitting.jl
Outdated
split(tmp::GeometryBasics.LineString,ax::GeoAxis) = split(tmp, ax.dest) | ||
|
||
function split(tmp::Vector{<:GeometryBasics.LineString},ax::GeoAxis) | ||
[split(a,ax.dest) for a in tmp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the lift
ing from the GeoAxis methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since ax.dest is an observable, isn't this sufficient?
split(tmp::GeometryBasics.LineString,dest::Observable) = @lift(split(tmp, $(dest)))
split(tmp::GeometryBasics.LineString,ax::GeoAxis) = split(tmp, ax.dest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An array of Observables probably won't be plottable by Makie - better to do the lifting as soon as possible IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you mean, but please go ahead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! I haven't been able to look at this in full detail yet, but had a few questions about return types etc.
Quick bump @gaelforget |
Co-authored-by: Haakon Ludvig Langeland Ervik <[email protected]>
Co-authored-by: Haakon Ludvig Langeland Ervik <[email protected]>
Co-authored-by: Haakon Ludvig Langeland Ervik <[email protected]>
do you mean adding a plot like the one below in the docs of GeoMakie? https://juliaocean.github.io/OceanRobots.jl/dev/examples/OceanOPS.html I should be able to do that in separate PR if you don't mind me adding OceanRobots.jl in docs/Project.toml |
all set with suggested revision I think |
Will review tomorrow, it's getting a bit late here unfortunately. Thanks for the update! |
Awesome! Thank you in advance |
Do you particularly care about the name here? I'm thinking of upstreaming this to GeometryOps, maybe in a method called This would allow the operation to be generalized to polygons (which need to be closed) and so on and so forth. Currently, the algorithm does not inject points at the line segment that is split and is probably not robust to floating point error - that's something that moving it to GeometryOps (or at least a GeometryOps-like workflow) can also fix. We can merge the PR with a couple of changes that I will push for now, but I would like to change the behaviour later, perhaps as a v0.8.0 after Juliacon. |
Name : seems to me that Piracy: not sure what's the issue here, or in what way this is Upstreaming this to GeometryOps : seems reasonable, happy to send a PR there later is that helps, but will need to look at GeometryOps first
Maybe I am confused but isn't it that objects in geojson and shape files are typically cut at antimeridian=+-180 (i.e., they are not closed polygons in the first place) which is why they plot nicely when lon0=0 ? One way to think about generalizing this would be
I don't know how GeometryOps does things or how you'd inject points, but from discussion in #128 with @haakon-e I was under the impression that adding points might complicate zooming in and out interactively with GLMakie, and that it might prevent reversibility (e.g. changing lon0 from 0 to -160 and back to 0 gradually via an observable lon0 you'd end up with a bunch more points at the end or something like that)
Sounds good |
Following up on #128
split
method from @haakon-eResulting plot should look as shown below.
ps. revised code should readily work with #207 , should be simple to merge (I tried)