Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.32%. Comparing base (e874c4a) to head (001b147).
⚠️ Report is 1518 commits behind head on main.

@@           Coverage Diff           @@
##             main    #2379   +/-   ##
=======================================
  Coverage   84.31%   84.32%           
=======================================
  Files         380      380           
  Lines       62217    62254   +37     
  Branches    12207    12217   +10     
=======================================
+ Hits        52461    52498   +37     
  Misses       9756     9756           

The time complexity of igraph_get_all_eids_between() should be investigated and included in the docs. It should be logarithmic due to the binary search, but logarithmic in what?

Either way, I suspect this will be too slow. Technically, this function will always take time proportional to the number of matrix elements due to igraph_matrix_null(). But in practice igraph_matrix_null() is extremely fast, and won't dominate the timings. What matters is the other operations.

This particular solution does as many edge lookups as the number of matrix elements, i.e. $N_\text{from} \times N_\text{to}$.

Nevertheless, this implementation is so simple that we can have high confidence in it. Thus let's use it to expand the tests before moving on to implementing a faster one.

What I proposed in the issue using sorted intersections should take time $N_\text{from} \times \langle d \rangle_\text{from}$ where $\langle d \rangle$ denotes the mean degree. I expect that in most practical cases $N_\text{to} >> \langle d \rangle_\text{from}$, but this is not necessarily the case, especially for small submatrices.

So it will be useful to think about various types of applications. Will the submatrices be smaller or larger than the typical degree?

@szhorvat So, am I right to assume in light of your comments that you are in favour of merging this PR even if you have concerns about the performance?

@GroteGnoom is this still a draft or is it ready to be merged?

No, I think we should fix the performance.

It is not ready, loop edges are not handled. Although that would be easy with an if (from == to) in the inner loop. LOOPS_ONCE vs LOOPS_TWICE also needs to be handled.

And we need docs.

It is not ready, loop edges are not handled.

I think they are? (the TODO can be removed I think)

But it's still a mess, so I'll at least clean up first. And I don't mind looking at performance either.

I think they are? (the TODO can be removed I think)

Sorry about that, I didn't look carefully enough.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.