refactor: remove for loops from weighted dense matrix creation (#1483)

[schochastics]

This PR removes (nested) for loops for dense weighted matrices. Specifically:

• get.adjacency.dense when attr != NULL
• graph.incidence.dense when weighted = TRUE
• get.incidence.dense when attr != NULL

[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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.

---

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.

[maelle]

Thank you!! I'll start by adding some tests (from examples) for as_adjacency_matrix() and the other functions impacted by this PR in another/other PR(s), then we can rebase this. I have to work on tests anyway.

[maelle]

I found a related issue #1137 cc @szhorvat

[krlmlr]

Thanks, great! The second commit was hard for me to understand, maybe we could create three PRs instead and review independently?

@maelle has #1539 that might improve coverage here. I'm rebasing, so let's check what the test coverage is, and adapt as needed.

[krlmlr]

Is this the same as

el <- cbind(idx, incidence[idx] + n1)

?

In either case, this needs a bit of explanation in a comment.

[schochastics]

Yes that is the same. I can refactor a bit and add comments

[schochastics]

sorry I was wrong here. It is not the same, which makes it obvious that comments are needed...

[maelle]

The tests are erroring, is it the tests' or the code's fault? 😅

[schochastics]

I will have a look!

[schochastics]

cc @maelle

rigraph/tests/testthat/test-conversion.R

Lines 175 to 183 in 0d96334

	expect_equal(
	weight_adj_matrix,
	matrix(
	c(0, 3.4, 0, 0, 1.2, 2.7, 0, 4.3, 0, 0, 6, 0, 0, 0, 0.1, 0),
	nrow = 4L,
	ncol = 4L,
	dimnames = list(c("a", "b", "c", "d"), c("a", "b", "c", "d"))
	)
	)

Here is an error in the test. The test graph has multiple edges and the edge weights are aggregated. E.g. the entry [3,3] is supposed to be 5.6+6.0=11.6 and not 6.0 and [4,2] is 6.1+ 3.3+4.3=13.7 and not 4.3. The aggregation makes sense and is invoked by use.last.ij <- FALSE in get.adjacency.sparse()

The later snapshot errors seem to be related with this error (I am not familiar with expect_snapshot). Do you want me to fix the tests in this PR, or do you do this in another branch?

[maelle]

Thank you! It'd make sense to fix them in another PR to main to check they're passing for main, then we'll merge and rebase this PR. At least I think it makes sense 😅

[schochastics]

hmm they currently pass on main, so in the current igraph version, get.adjacency.dense does not aggregate multiple edge weights (but get.adjacency.sparse does). So I guess if you change this test on main, it will fail. But IMHO this is a bug. dense and sparse should give the same result and both aggregate

[maelle]

then either open a PR (and we'd cherry-pick the commit into this branch here) or an issue for discussion? Thanks so much.