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

feat: Avoid vertex and edge sequences in attributes #808

Merged
merged 4 commits into from
May 28, 2023
Merged

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented May 21, 2023

For #807.

@krlmlr
Copy link
Contributor Author

krlmlr commented May 21, 2023

This is branched off of v1.4.2, running revdepchecks now.

@krlmlr krlmlr requested a review from ntamas May 22, 2023 04:17
@krlmlr
Copy link
Contributor Author

krlmlr commented May 22, 2023

No problems at all!

https://github.com/igraph/rigraph/pull/808/files#diff-bbdda7f28391ee0ec001638e5603609582124345e0d65ebd4747cc592f9bd56dR1

I suggest we include this into the upcoming 1.4.3 release, to avoid problems like #807.

@krlmlr krlmlr changed the title Avoid vertex and edge sequences in attributes feat: Avoid vertex and edge sequences in attributes May 22, 2023
mbojan added a commit to mbojan/netseg that referenced this pull request May 22, 2023
This anticipates igraph changes as described in #16 and
igraph/rigraph#808.
@szhorvat
Copy link
Member

I suggest we include this into the upcoming 1.4.3 release, to avoid problems like #807.

Since 1.4.3 is published, how about a 1.4.4? If you do go ahead with a 1.4.4, can you please update the C core to the latest commit on this branch, to include an additional fix? The commit message explains the fix. https://github.com/igraph/igraph/tree/release/0.9

@szhorvat
Copy link
Member

szhorvat commented May 24, 2023

Ah, I see these commits were already included. I was confused by looking at the LTO checks on CRAN, but those are outdated, still showing information not even for 1.4.2, but for 1.4.1? Why would that be so @krlmlr, and is it something we should be concerned about?

https://www.stats.ox.ac.uk/pub/bdr/LTO/igraph.out

Notice this warning:

core/community/leading_eigenvector.c:156:12: warning: ‘igraph_i_community_leading_eigenvector2’ defined but not used [-Wunused-function]

It's clear from looking at line 156 of the mentioned source file that this warning is for version 1.4.1 or earlier.

@krlmlr
Copy link
Contributor Author

krlmlr commented May 27, 2023

CRAN keeps asking if LTO issues have been fixed, I keep telling them that we fix in igraph > 1.4.99 because of the C library update. Nothing to worry about for now.

@szhorvat
Copy link
Member

szhorvat commented May 27, 2023

These are not real issues. They are invalid warnings from GCC. As far as I'm concerned, this is a GCC bug.

I don't see any disadvantage-free way to make these GCC warnings go away.

@krlmlr
Copy link
Contributor Author

krlmlr commented May 27, 2023

I've seen CRAN in one instance accept LTO warnings as GCC bugs, we'll see.

@krlmlr krlmlr marked this pull request as draft May 27, 2023 11:50
@krlmlr
Copy link
Contributor Author

krlmlr commented May 27, 2023

Need to double-check if this is really needed or if perhaps there might be other ways to resolve this.

@szhorvat
Copy link
Member

szhorvat commented May 27, 2023

With igraph 0.10, which has been significantly cleaned up, it will be much easier to show that these are likely a GCC problem.

@krlmlr krlmlr marked this pull request as ready for review May 28, 2023 07:23
@krlmlr
Copy link
Contributor Author

krlmlr commented May 28, 2023

After reviewing `[.igraph.vs`(), I am now confident that this is a good solution.

@mbojan: We do support arbitrary vectors, but vertex and edge sets create an unhealthy back reference that our code isn't prepared to deal with.

@krlmlr krlmlr merged commit 5008105 into main May 28, 2023
@krlmlr krlmlr deleted the f-avoid-seq-attrs branch May 28, 2023 08:12
@mbojan
Copy link

mbojan commented May 28, 2023

Thanks @krlmlr , that definitely makes sense to me. I already tried the version from the PR 👍

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