Skip to content

fix: revdepcheck results#789

Merged
krlmlr merged 16 commits intomainfrom
f-igraph-t-idx-revdepcheck
Jun 15, 2023
Merged

fix: revdepcheck results#789
krlmlr merged 16 commits intomainfrom
f-igraph-t-idx-revdepcheck

Conversation

@krlmlr
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr commented Apr 21, 2023

for 6222e73.

This PR contains the results from checking all reverse dependencies against 6222e73. This is the commit where all indexes in the internal list have been shifted. It seems like our code (or perhaps even other packages) are relying on those indexes. We need to understand what causes the failures, and fix them.

@krlmlr krlmlr added this to the upgrade milestone Apr 21, 2023
@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Apr 21, 2023

@Antonov548: See https://github.com/igraph/rigraph/blob/f-igraph-t-idx-revdepcheck/revdep/problems.md for details on the errors. This file shows the result from running rcmdcheck::rcmdcheck() for downstream packages with igraph 6222e73, compared against the released version of igraph.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Apr 21, 2023

This branch is based on 6222e73, please don't merge the main branch into this branch for now.

@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Apr 21, 2023

Some of these failures may be occurring because the tests of some packages load existing igraph data, which does not follow the changed storage order that's being tested here.


It seems to me that dealing with this may not be simple. The format version could be bumped, and upgrade_graph() could be adapted. That's not little work. Is it worth it at all?

@Antonov548
Copy link
Copy Markdown
Contributor

Antonov548 commented Apr 24, 2023

It seems to me that dealing with this may not be simple.

I checked approximately 7 cases from the list and seems it's something related to loading old graph, since this logic is covered with the tests in igraph and sucsesuflly passed.
Should I really go trough all of them? What do you think? Because in some cases it's really not simple.

@ntamas

@ntamas
Copy link
Copy Markdown
Member

ntamas commented Apr 24, 2023

As @szhorvat said, most of these failures are probably caused by the tests trying to load an old graph from a saved Rdata file. In these files, the components of the list representing the graph are in different order than the shuffled order in our new codebase and there's no migration code in place that would rearrange them in the right order.

OTOH it's really hard to distinguish genuine failures in the revdepcheck output from failures caused by the lack of migration code, so if we really want to make use of the revdepcheck result, we should probably come up with the code necessary to convert old saved data in test cases to the new order of items in the list.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 16, 2023

I started a new round of checks with #799, will push here.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 19, 2023

@Antonov548: The problems at https://github.com/igraph/rigraph/blob/f-igraph-t-idx-revdepcheck/revdep/problems.md look serious, I didn't expect those. Can you please take a look?

@Antonov548
Copy link
Copy Markdown
Contributor

Can you please take a look?

Thanks. I will take a look.

@Antonov548
Copy link
Copy Markdown
Contributor

Antonov548 commented May 21, 2023

I checked packages. Seems the really problem related to changes of graph structure are in the leidenbase package. Other seems not related to this change. Also some examples could be outdated.

  • causaloptim : load invalid graph
  • clustAnalytics: needs an update of igraphdata
  • leidenbase : has copy of R_SEXP_to_igraph
  • leidenAlg : use leidenbase
  • nda : use leidenAlg
  • SNscan : expected error to update graph

  • drake : ?
  • netseg : ? (could be something related to structure)
  • shazam : ?
  • migraph : related to changes of algorithms in igraph?
  • metanetwork : related to changes of algorithms in igraph?

@szhorvat
Copy link
Copy Markdown
Member

  • migraph : related to changes of algorithms in igraph?

  • metanetwork : related to changes of algorithms in igraph?

But this PR doesn't change the igraph version, does it? It merely changes the internal layout of the igraph objects. So there are no changes in algorithms.

@Antonov548
Copy link
Copy Markdown
Contributor

But this PR doesn't change the igraph version, does it? It merely changes the internal layout of the igraph objects. So there are no changes in algorithms.

I was just wondering if they really didn't update the version of the rigraph relative to the one used in the package we're testing.

Then I will try to check why the values are slightly different in the tests.

@szhorvat
Copy link
Copy Markdown
Member

  • leidenAlg : use leidenbase

Did you mean to say that leidenAlg uses leidenbase? That is not so, but leidenAlg does have a partial copy of the igraph code, and it does access the graph object internals as well, so the failure is expected.

@Antonov548
Copy link
Copy Markdown
Contributor

  • leidenAlg : use leidenbase

Did you mean to say that leidenAlg uses leidenbase? That is not so, but leidenAlg does have a partial copy of the igraph code, and it does access the graph object internals as well, so the failure is expected.

Ah, sorry. It's typo. Originally it was that both have igraph code.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Running another set of checks with #805. Expecting much fewer failures.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Sent an e-mail to the SNscan maintainers, they need to fix that.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

I'll take a look at gemtc and netseg, the leiden* packages are the only ones remaining.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

netseg reprex:

netseg::ssi(netseg::EF3, "race")
#> Error in igraph::induced.subgraph(g, which(ids == b)): At core/core/vector.pmt:1030 : Assertion failed: v->stor_begin != NULL. This is an unexpected igraph error; please report this as a bug, along with the steps to reproduce it.
#> Please restart your R session to avoid crashes or other surprising behavior.

Created on 2023-05-21 with reprex v2.0.2

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

netseg gives me headaches. Need to sleep over it.

options(conflicts.policy = list(warn = FALSE))
library(netseg)
ssi(EF3, "race")
#> Error in igraph::induced.subgraph(g, which(ids == b)): At core/core/vector.pmt:1030 : Assertion failed: v->stor_begin != NULL. This is an unexpected igraph error; please report this as a bug, along with the steps to reproduce it.
#> Please restart your R session to avoid crashes or other surprising behavior.

Created on 2023-05-21 with reprex v2.0.2

Even with upgrade_graph() :

options(conflicts.policy = list(warn = FALSE))
library(netseg)

g <- igraph::upgrade_graph(EF3)
ssi(g, "race")
#> Error in igraph::induced.subgraph(g, which(ids == b)): At core/core/vector.pmt:1030 : Assertion failed: v->stor_begin != NULL. This is an unexpected igraph error; please report this as a bug, along with the steps to reproduce it.
#> Please restart your R session to avoid crashes or other surprising behavior.

Created on 2023-05-21 with reprex v2.0.2

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Can't test gemtc on macOS, will go to Ubuntu for that.

@szhorvat
Copy link
Copy Markdown
Member

Is the netseg error happening only with the reshuffled igraph data structure, or also with 1.4.2 / 1.4.3?

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

All the failures we're seeing here are regressions compared to igraph 1.4.2.

@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented May 21, 2023

@krlmlr Do I understand it correctly that these revdepchecks are done with the reshuffled version of the igraph data structure? Or is it just the dev version of igraph?

I see that you already opened an issue for netseg. But this is a fatal error, which should be impossible to trigger from R/igraph. In other words, if it happens, it's always our fault.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

This is with an igraph object loaded from storage, and performing some operations on it. Could be off-label use too, I just don't know. We'll see.

@szhorvat
Copy link
Copy Markdown
Member

off-label use

What does this mean?

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Usage patterns that we don't fully support. Just because something works in 1.4.2 doesn't mean it's necessarily part of the API.

Not suggesting this is the case, but could be.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented Jun 11, 2023

Started another full round of revdepchecks.

@krlmlr krlmlr merged commit 4a0253d into main Jun 15, 2023
@krlmlr krlmlr deleted the f-igraph-t-idx-revdepcheck branch June 15, 2023 05:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants