refactor: consolidate graph.incidence.* (#1483)

[schochastics]

This PR removes (nested) for loops for graph.incidence.dense() when weighted = TRUE

(cherry picked commits from #1518)

This PR removes graph.incidence.sparse() and graph.incidence.dense() and replaces it with a general graph.incidence.build() and more specialized helper functions.

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[schochastics]

not sure why it fails but I am checking it.
I think I switched from row-first to column-first in the output.

[krlmlr]

Thanks. Overall, the code doesn't seem to be well-prepared for the edge case of one-row matrices, we might need drop = TRUE in several places.

Could we perhaps forward to graph.incidence.sparse() in this case, perhaps even change graph.incidence.sparse() to accept both sparse and dense matrices? A lot of code there looks similar to what we have here.

[schochastics]

Yes I now realize that I borrowed the logic from the sparse function and simply adapted where needed for the dense case. Thats not optimal. I think I can refactor both *.sparse() and *.dense() into one function.

[schochastics]

The logic is now completely rewritten. There are no dedicated graph.incidence.sparse and graph.incidence.dense anymore since there was a lot of code duplication. Helper functions are now disjoint and there is no duplication of code anymore

[szhorvat]

@schochastics I haven't been able to follow what's going on with the "incidence" (bipartite adjacency) matrix work, as I'm overwhelmed with other work at the moment. Do you think it would be good to have a short video meeting to discuss how the C library handled bipartite incidence matrices, so that when the R package finally starts using this functionality, the behaviour wouldn't change? I.e., just make sure that whatever's implemented in pure R now matches the C behaviour.

[schochastics]

@szhorvat Happy to do so. I sent you an email

[schochastics]

This PR can be reviewed now

[krlmlr]

Thanks, this might need yet another iteration or two.

[schochastics]

@krlmlr ready to be reviewed again

[krlmlr]

Can you please tweak and auto-merge?