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

Version 3.2.0, add Local IJ to docs #149

Merged
merged 4 commits into from
Oct 8, 2018

Conversation

isaacbrodsky
Copy link
Collaborator

The Distance docs are replaced with docs for the Local IJ group of functions.

The coordinate systems documentation page is converted to use the expected single asterisk for italics, rather than underscores which aren't understood by Ocular.

@coveralls
Copy link

coveralls commented Oct 4, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling afdc74c on isaacbrodsky:version-3.2.0 into 36a2ceb on uber:master.

axes are 120° apart. The origin index might not be at
(0, 0) in the local coordinate system.

LOcal IJ coordinates are only valid within a certain radius of the origin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: LOcal -> Local


Produces local IJ coordinates for an index anchored by an origin.

This function is experimental, and its output is not gauranteed
Copy link
Collaborator

Choose a reason for hiding this comment

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

gauranteed -> guaranteed


Produces an index for local IJ coordinates anchored by an origin.

This function is experimental, and its output is not gauranteed
Copy link
Collaborator

Choose a reason for hiding this comment

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

gauranteed -> guaranteed

int experimentalLocalIjToH3(H3Index origin, const CoordIJ *ij, H3Index *out);
```

Produces an index for local IJ coordinates anchored by an origin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Might read better as Produces an H3 index from local IJ coordinates anchored by an origin.

Copy link
Collaborator

@dfellis dfellis left a comment

Choose a reason for hiding this comment

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

Minor typos and one small suggestion.

This function is experimental, and its output is not guaranteed
to be compatible across different versions of H3.

## h3Distance
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a little tricky. h3Distance is a generally applicable function that many users may want without understanding a concept of local coordinates. I'm a little worried that we're burying it here because its implementation is based on IJ coords, rather than putting it somewhere that would make more sense to the reader.

Would it make more sense, for example, to rename Neighbors to Traversal and put this there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, organizing this is not easy. While they are not connected from a use perspective, these functions have a similar set of limitations, which I wanted to make the user aware of.

I feel like if Neighbors is renamed to Traversal that could also encompass the Local IJ functions. Maybe all of them should be on the same page?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I'd support that.

Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

Looks good!

@isaacbrodsky isaacbrodsky merged commit 4b93211 into uber:master Oct 8, 2018
@isaacbrodsky isaacbrodsky deleted the version-3.2.0 branch October 8, 2018 17:50
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
Version 3.2.0, add Local IJ to docs
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

4 participants