luukvdmeer / sfnetworks

Tidy Geospatial Networks in R
https://luukvdmeer.github.io/sfnetworks/
Other
345 stars 20 forks source link

Update test failing on r-devel #229

Closed loreabad6 closed 1 year ago

loreabad6 commented 1 year ago

Due to CRAN errors on r-devel that I find hard to debug with the time limit, I commented out the test causing the issue. My impression is that the match algorithm from base R is going to change in the next r version and expect_setqueal(), which relies on it, does not support anymore sfnetwork objects. We can think of another way to perform that test in any case, since I think it could be better structured.

agila5 commented 1 year ago

Hi! I briefly checked the problem and I think the error was created from the following commit: https://github.com/wch/r-source/commit/e56fd312afd51775a162e22a29249164bc74810c

In fact, now on R-devel we get the following:

library(sfnetworks)
sfn <- as_sfnetwork(roxel)

mtfrm(sfn)
#> Error in mtfrm.default(sfn): cannot mtfrm

Created on 2022-10-16 with reprex v2.0.2

The problem is that mtfrm() is (somehow) internally called by match() (as you said). I don't understand the reasoning behind that commit but, IMO, if the object of our test is to check that the two networks are equal, maybe we could run the following:

expect_true(igraph::identical_graphs(st_network_join(net, rdm_net), st_network_join(net, rdm))))

We could also define a new function (i.e. identical_sfnetwork()) as a wrapper around identical_graphs (with extra checks for CRS).

codecov-commenter commented 1 year ago

Codecov Report

Base: 67.99% // Head: 67.99% // No change to project coverage :thumbsup:

Coverage data is based on head (d6257a5) compared to base (9ac2842). Patch has no changes to coverable lines.

:exclamation: Current head d6257a5 differs from pull request most recent head e2bce40. Consider uploading reports for the commit e2bce40 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #229 +/- ## ======================================= Coverage 67.99% 67.99% ======================================= Files 21 21 Lines 1584 1584 ======================================= Hits 1077 1077 Misses 507 507 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

loreabad6 commented 1 year ago

Thank you @agila5, I integrated your suggestion for now, to try and resubmit asap to CRAN. We can think later indeed of a wrapper. @luukvdmeer, since tests passed now on GHA as well I will push to this same branch the NEWS.md, DESCRIPTION and cran-comments.md files. Sounds good?