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

add [inv|fwd]_intermediate() - to calculate intermediate points (like improved Geod.npts()) #841

Merged
merged 6 commits into from
May 17, 2021

Conversation

idanmiara
Copy link
Contributor

@idanmiara idanmiara commented May 5, 2021

pyproj/geod.py - add a parameter return_points to Geod.npts() to determinate the return type; improve return type hint
pyproj/geod.py - add a parameters initial_idx, terminus_idx to Geod.npts()
pyproj/[geod.py, _geod.pyi, _geod.pyx] - add inv_intermediate_by_npts functions that uses given buffers out_lons, out_lats (and out_azis) buffers instead of creating new buffers
pyproj/_geod.pxd - add interface for geod_lineinit
pyproj/[geod.py, _geod.pyi, _geod.pyx] - add inv_intermediate_by_del, fwd_intermediate_by_npts, fwd_intermediate_by_del functions, similar to inv_npts but using forward calculation
test/test_geod.py - add tests for new functions

Related PR

#825

@idanmiara
Copy link
Contributor Author

idanmiara commented May 5, 2021

Maybe I'll change this flag into an tuple of Enum so other return types could be supported in the future, like (points, lons, lats, ranges, azimuths)

@snowman2
Copy link
Member

snowman2 commented May 7, 2021

@idanmiara, I am planning on doing a release soon since PROJ 8.0.1 was just released. Do you want to try to include this in this release or do you need more time?

@idanmiara
Copy link
Contributor Author

@idanmiara, I am planning on doing a release soon since PROJ 8.0.1 was just released. Do you want to try to include this in this release or do you need more time?

Hi, thanks for your letting me know!
I'm trying to complete it now.

@idanmiara
Copy link
Contributor Author

idanmiara commented May 7, 2021

I had a few other related ideas, I've implemented them in geod.py but I see that I can probably implement them much more efficiently in Cython by modifying _geod.Geod._npts:

  1. add a flag add_points: int add an option to add initial and/or terminus points to the output vector (0= default- add neither, 1=add first; 2=add last; 3= add both)
  2. add an optional az: float parameter to allow to calculate the line by using az instead of lon2, lat2 using geod_lineinit (like before REF: Update pyproj.Geod.npts to use geod_inverseline #825)
  3. add optional del_s: float and n_ceil: bool that can be an alternative way to calculate the number of points number (https://github.com/pyproj4/pyproj/pull/825/files#diff-884caf2af6e10fc8e0b305d38b427edc7b43d9ed3ec739aed1be2de94049ab7dR192):
    npts = dist / del_s
    if n_ceil:
        npts = math.ceil(npts)
    npts = int(npts)

    dist = npts * del_s
  1. optionally add a pazi2 vector to the output (https://github.com/pyproj4/pyproj/pull/825/files#diff-884caf2af6e10fc8e0b305d38b427edc7b43d9ed3ec739aed1be2de94049ab7dR196) by adding another return type option

I've never used Cython before. from what I see it's not too complicated. Do you have some pointers on how to start besides the Cython official tutorials? How do you debug it?

I'm thinking if I can make it to the next release, when are you planning on making it?

@snowman2
Copy link
Member

snowman2 commented May 7, 2021

Do you have some pointers on how to start besides the Cython official tutorials? How do you debug it?

That sounds like a good place to start. Also, looking through the existing code could be helpful.

For debugging, I usually rebuild the cython after each change with:

pip install -e . or python setup.py build_ext --inplace

And then test it like normal python code.

@snowman2
Copy link
Member

snowman2 commented May 7, 2021

I'm thinking if I can make it to the next release, when are you planning on making it?

No rush. If you are regularly working on it, it can wait until you are done.

@snowman2
Copy link
Member

snowman2 commented May 7, 2021

One thought I had was to potentially break out the different types of inputs/responses into different methods. This will add clarify for expected input/output to the user and simplify the logic for each method.

@idanmiara idanmiara force-pushed the npts_return branch 9 times, most recently from d5acdf3 to 8098312 Compare May 9, 2021 14:23
@idanmiara idanmiara changed the title geod.py - add a flag return_points to Geod.npts() to determinate the return type Geod.npts() - add many options May 9, 2021
@idanmiara idanmiara force-pushed the npts_return branch 11 times, most recently from a899842 to 527c0b5 Compare May 10, 2021 11:22
@idanmiara idanmiara force-pushed the npts_return branch 2 times, most recently from f8ef2ca to 9fc888f Compare May 14, 2021 04:04
pyproj/geod.py Outdated Show resolved Hide resolved
@snowman2
Copy link
Member

snowman2 commented May 15, 2021

I see a commit where you say: ".. versionadded:: 3.1.0" with ".. versionadded:: 3.1" for consistency that modifies the crs/transformer files. Would you mind moving this to a separate PR for clarity? And, if you are feeling extra motivated, you could do a search for versionadded and make all of them consistent in that PR, but definitely not necessary.

@snowman2
Copy link
Member

@idanmiara thanks for your patience with this PR! I think it is very close to being ready for merge.

@idanmiara
Copy link
Contributor Author

I see a commit where you say: ".. versionadded:: 3.1.0" with ".. versionadded:: 3.1" for consistency that modifies the crs/transformer files. Would you mind moving this to a separate PR for clarity? And, if you are feeling extra motivated, you could do a search for versionadded and make all of them consistent in that PR, but definitely not necessary.

Sure.

@idanmiara
Copy link
Contributor Author

@idanmiara thanks for your patience with this PR! I think it is very close to being ready for merge.

Thank you, I highly appreciate your thorough review!

…od.npts()

test/test_geod.py - test the new functionality
…ions that uses given buffers `out_lons`, `out_lats` (and `out_azis`) buffers instead of creating new buffers

test/test_geod.py - add tests for `inv_intermediate`
@idanmiara idanmiara force-pushed the npts_return branch 4 times, most recently from c26a239 to f99cee6 Compare May 17, 2021 14:43
pyproj/_geod.pyx Outdated Show resolved Hide resolved
pyproj/geod.py Outdated Show resolved Hide resolved
…_intermediate` (improved version of `npts()`) functions; add `out_lons`, `out_lats` params to `npts()`; add examples and some more docs

pyproj/enums.py, docs/api/enums.rst - add `GeodIntermediateFlag` flag
test/test_geod.py - add tests for new functions
@snowman2
Copy link
Member

@idanmiara this looks good. Ready to merge?

@idanmiara
Copy link
Contributor Author

@idanmiara this looks good. Ready to merge?

Absolutely!

@snowman2
Copy link
Member

Thanks @idanmiara 👍

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.

2 participants