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

initial implementation of linetrace #24

Merged
merged 5 commits into from
Nov 21, 2023
Merged

Conversation

alpha-beta-soup
Copy link
Contributor

What this PR does

cf #11 for supporting linestrings. This is a work-in-progress and needs some deliberation.

I tried to come up with an equivalent name to "polyfill", and came up with "linetrace". Happy to change it.

There's a number of ways this could be implemented, depending on a few inter-related criteria:

  • Should order be retained?
  • Should there be repeating cells (in sequence) in the linetrace output?
  • Should there be repeated cells (at all) in the linetrace output?
  • Are parts of multilinestrings relevant?
  • Should there be an equivalent to explode?
  • What should explode do with a multilinestring? (Perhaps a MultiIndex?)

In general I think that the return type should be an ordered list, not a set. However I don't permit repeated cells if they are in sequence. Non-sequential repeats are possible to handle cases like self-intersection.

This currently supports multilinestring input, but information about the parts is not retained. I thought about an extra parameter to handle these, maybe retain_parts, True by default.

Happy to take direction on the appropriate behaviour, but at some level there's probably an element of opinion, unless there's a standard (even a de facto one) for how linestrings should be represented in a DGGS---if the idea makes sense at all.

@alpha-beta-soup
Copy link
Contributor Author

Both explode and index_parts are now supported. Behaviour is hopefully clear through tests.

@DahnJ I'd appreciate a review; at this point I've implemented everything I care about for this feature.

h3pandas/h3pandas.py Outdated Show resolved Hide resolved
@DahnJ
Copy link
Owner

DahnJ commented Nov 9, 2023

Hi @alpha-beta-soup thanks so much for the contribution! Sorry for my late reply on my side, I have been busy with other things.

I think the PR looks great, thanks for providing the tests and submitting such clean code in general!

With regards to your design choices, I think they all make sense. Since you're the first to request this feature, I would hold off from implementing anything like retain_parts unless someone requests it.

Just had a comment about index_parts, otherwise this already looks great ⭐

Copy link
Owner

@DahnJ DahnJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now, thanks a lot for the contribution @alpha-beta-soup !

@DahnJ DahnJ merged commit 258e868 into DahnJ:master Nov 21, 2023
@DahnJ DahnJ mentioned this pull request Nov 21, 2023
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.

None yet

2 participants