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

improvements and example for linesplitting #228

Merged
merged 13 commits into from
Jul 10, 2024

Conversation

gaelforget
Copy link
Contributor

Following up on #128

  • merge improved split method from @haakon-e
  • add missing methods
  • revise docs section, now incl. working example

Resulting plot should look as shown below.

ps. revised code should readily work with #207 , should be simple to merge (I tried)

Screenshot 2024-05-20 at 12 02 32 AM

@gaelforget
Copy link
Contributor Author

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

@lazarusA
Copy link
Contributor

ohh... this is a good use case. If will be great to have it.

Copy link
Contributor

@haakon-e haakon-e left a 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 lifting is necessary for this.

I left a few comments and suggestions in the docs that should be equivalent to what you wrote.

docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
Copy link
Member

@asinghvi17 asinghvi17 left a 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 Show resolved Hide resolved
src/linesplitting.jl Outdated Show resolved Hide resolved
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]
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@asinghvi17 asinghvi17 left a 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.

@asinghvi17
Copy link
Member

Quick bump @gaelforget

gaelforget and others added 4 commits July 3, 2024 05:19
Co-authored-by: Haakon Ludvig Langeland Ervik <45243236+haakon-e@users.noreply.github.com>
Co-authored-by: Haakon Ludvig Langeland Ervik <45243236+haakon-e@users.noreply.github.com>
Co-authored-by: Haakon Ludvig Langeland Ervik <45243236+haakon-e@users.noreply.github.com>
@gaelforget
Copy link
Contributor Author

ohh... this is a good use case. If will be great to have it.

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

@gaelforget
Copy link
Contributor Author

all set with suggested revision I think

@asinghvi17
Copy link
Member

Will review tomorrow, it's getting a bit late here unfortunately. Thanks for the update!

@gaelforget
Copy link
Contributor Author

Will review tomorrow, it's getting a bit late here unfortunately. Thanks for the update!

Awesome! Thank you in advance

@asinghvi17
Copy link
Member

asinghvi17 commented Jul 4, 2024

Do you particularly care about the name here? I'm thinking of upstreaming this to GeometryOps, maybe in a method called cut_antimeridian. That also gets rid of the "piracy" (relatively benign, but not that great nevertheless).

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.

@gaelforget
Copy link
Contributor Author

gaelforget commented Jul 5, 2024

Do you particularly care about the name here? I'm thinking of upstreaming this to GeometryOps, maybe in a method called cut_antimeridian. That also gets rid of the "piracy" (relatively benign, but not that great nevertheless).

Name : seems to me that split with a keyword for lon, lon0, or longitude + a reference to antemeridian in the docstring would be nicer, and makes sense since this operation is a direct analogy to what Base.split does. Also cut_antimeridian is kind of confusing (cut_at_antimeridian?) and long.

Piracy: not sure what's the issue here, or in what way this is piracy, aside from the fact that split is a common word that applies to strings amongst many other things.

Upstreaming this to GeometryOps : seems reasonable, happy to send a PR there later is that helps, but will need to look at GeometryOps first

This would allow the operation to be generalized to polygons (which need to be closed) and so on and so forth.

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 split(x,polygon=p) where p defines a closed region on the surface of the Earth or on the projection plane (e.g. the GeoAxis). But there are probably several ways to look at this, e.g. depending if one takes geo to mean geometry, geography, or geospatial.

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.

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)

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.

Sounds good

@asinghvi17 asinghvi17 merged commit 4e39e7e into MakieOrg:master Jul 10, 2024
1 of 2 checks passed
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

Successfully merging this pull request may close these issues.

4 participants