BUG: refactor computation of angles in COINS

[anastassiavybornova]

Based on #629 (solved) and #630 (superseded)
Refactor computation of interior angle between 2 lines.

Previously, the interior angle between 2 lines was computed by:

1. computing each line's angle formed with the horizontal plane
2. computing each line's orientation relative to the horizontal plane
3. applying trigonometric formulas while distinguishing different use cases (of orientation combinations)

this is now simplified into making use of the fact that by definition, the two lines we're computing interior angles between will always start in the same point, hence we can reduce the entire problem to the computation of an angle between 2 vectors in 2dim space from the same origin, hence referencing the horizontal plane // orientation // direction becomes obsolete. this pr supersedes #630 in that orientation is dropped completely from the code.

just for the records, the bug mentioned in #629 resulted from the fact that the use case of sharp angles (<90deg) was not considered in the use case distinction.

thanks to @fnattino for the input!!

Fixes #629 and closes #630

[martinfleis]

Can we remove this assertion?

[anastassiavybornova]

We're accounting for the unwanted cases before that, so yes we can remove it

[codecov]

Codecov Report

Attention: Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.8%. Comparing base (4037c70) to head (43f4a16).
Report is 85 commits behind head on main.

Files with missing lines	Patch %	Lines
momepy/coins.py	88.2%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #632     +/-   ##
=======================================
+ Coverage   97.4%   97.8%   +0.4%     
=======================================
  Files         26      38     +12     
  Lines       4328    6215   +1887     
=======================================
+ Hits        4214    6079   +1865     
- Misses       114     136     +22     

Files with missing lines	Coverage Δ
momepy/tests/test_coins.py	100.0% <100.0%> (ø)
momepy/coins.py	99.0% <88.2%> (+3.7%)	⬆️

[anastassiavybornova]

one of (many!!) options to figure out which two of the four points in the vectors (a,b) and (c,d) are identical

[anastassiavybornova]

since the COINS algorithm operates with position-based (index-based) attributes of lines, the very first attribute being "orientation": this is now obsolete, so this list will always be empty (we're not computing orientation anymore), but keeping it there nevertheless for the moment not to mess up with the index-based accessing of attributes in the rest of the algorithm. (would probably require major refactoring if we wanted to get rid of this issue, for now i guess it's best just not to touch it)

[martinfleis]

One thing I am not super excited about is that this significantly affects performance. On the network from the user guide:

main: 277 ms ± 8.87 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
this PR: 545 ms ± 12.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

[fnattino]

(sorry for interfering - again)

One thing I am not super excited about is that this significantly affects performance. On the network from the user guide:

I have noticed a similar performance degradation when testing for #630 . I think it is the overhead of creating many very small numpy arrays, as this is carried for each of the line segments in the network. In the test I have run, using native python types and functions from math was much faster than bringing the 2-element-long lists to np.ndarray.

[martinfleis]

@fnattino thanks for the pointer! With 8e5bf56, the timing goes down to 316 ms ± 1.45 ms per loop (mean ± std. dev. of 7 runs, 1 loop each) which is acceptable slowdown, I guess.

[martinfleis]

And with a6fcacb to 255 ms ± 8.15 ms per loop (mean ± std. dev. of 7 runs, 1 loop each), so we're actually a little bit faster now :).

[jGaboardi]

@fnattino

(sorry for interfering - again)

You do not need to apologize and you are not interferring!

[martinfleis]

all good with me!

[jGaboardi]

How about this 310 error?

[martinfleis]

@jGaboardi present on main https://github.com/pysal/momepy/actions/runs/9688801724/job/26747565038 and has nothing to do with our code. xref #634