Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Track revdepcheck results #621

Merged
merged 1 commit into from Jan 16, 2023
Merged

chore: Track revdepcheck results #621

merged 1 commit into from Jan 16, 2023

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Jan 3, 2023

Only three packages failing.

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #621 (66fef30) into dev (d9cc1a8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #621   +/-   ##
=======================================
  Coverage   62.69%   62.69%           
=======================================
  Files          73       73           
  Lines       21530    21530           
=======================================
  Hits        13499    13499           
  Misses       8031     8031           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ntamas
Copy link
Member

ntamas commented Jan 3, 2023

r6causal imports causaleffect so I strongly suspect that the two failures are related.
As for sfnetwork, the output does not seem to provide any useful clues :(

@ntamas
Copy link
Member

ntamas commented Jan 3, 2023

Also, is this without the change proposed to avoid recycling of attribute values?

@szhorvat
Copy link
Member

szhorvat commented Jan 3, 2023

As for sfnetwork, the output does not seem to provide any useful clues :(

It references distances() with the Johnson method, which we did change.

   10. └─sfnetworks:::st_network_cost.sfnetwork(...)
   11.   ├─base::do.call(igraph::distances, c(args, dots))
   12.   └─igraph (local) `<fn>`(`<sfnetwrk>`, 1, 10, weights = `<[m]>`, mode = "in", algorithm = "johnson")

Right now it only supports mode='out' with directed graphs, otherwise it throws an error. See #571 for why this change was necessary. Here they use mode='in'. So that's the reason for the failure.

But I have to say that the error message is very confusing:

> g<-sample_gnm(10,20,directed=T)
> distances(g,algorithm='johnson')
Error in distances(g, algorithm = "johnson") : 
  Johnson's algorithm works with mode="out" only for directed graphs
  • It's unclear that the problem occurred because the default was mode='all'. BTW that's a pretty bad default for distances() ... I'd expect that in directed graphs, directed distances will be computed by default. But I guess this can't be fixed as it would be a "breaking change".
  • When I read the message, it sounded as if the word "only" referred to "directed". In fact, it refers to the mode setting. It should probably say, "Johnson's algorithm only supports mode="out" when using directed graphs."

@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 3, 2023

Yes, this is the baseline.

@szhorvat
Copy link
Member

szhorvat commented Jan 3, 2023

It might make sense to implement mode='in' for Johnson on the R side. All it really takes is swapping the from/to arguments an transposing the result matrix. Thoughts?

@szhorvat
Copy link
Member

szhorvat commented Jan 3, 2023

It's good to note that the test sfnetwork uses is somewhat pointless, as it's done on a graph with non-negative edge weights. Johnson's algorithm is only useful with directed graphs that have some negative weights. If the graph is not like this, the C function just calls the Dijkstra implementation anyway.

@ntamas
Copy link
Member

ntamas commented Jan 3, 2023

All it really takes is swapping the from/to arguments an transposing the result matrix. Thoughts?

Does it? Wouldn't we also need to reverse the edge directions?

@szhorvat
Copy link
Member

szhorvat commented Jan 3, 2023

That function only returns distances, not paths. The A-to-B distance with "out" is the same as the B-to-A distance with "in", no? With "in" we go against the arrows in A--->B.

> g<-sample_gnm(10,20,directed=T)
> all(t(distances(g,mode='in')) == distances(g,mode='out'))
[1] TRUE

@krlmlr krlmlr changed the title revdepcheck results chore: Track revdepcheck results Jan 16, 2023
@krlmlr krlmlr merged commit 381c8b9 into dev Jan 16, 2023
@krlmlr krlmlr deleted the f-revdep branch January 16, 2023 05:01
@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 16, 2023

Merging to add infrastructure to track revdepcheck results on GitHub.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 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.

None yet

3 participants