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

refactor: use 'better' logic for updating vertex attributes #1330

Merged
merged 2 commits into from
Jul 4, 2024
Merged

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Mar 29, 2024

Fix #1327

@maelle maelle added the upkeep maintenance, infrastructure, and similar label Mar 29, 2024

This comment was marked as outdated.

R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
@krlmlr krlmlr marked this pull request as ready for review April 9, 2024 13:17
@krlmlr krlmlr marked this pull request as draft April 9, 2024 13:18
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good.

@clpippel: what do you think?

R/attributes.R Show resolved Hide resolved
@krlmlr krlmlr changed the title refactor: use 'better' logic refactor: use 'better' logic for updating vertex attributes May 23, 2024
@maelle maelle added this to the 2.0.4 milestone May 23, 2024
@maelle
Copy link
Contributor Author

maelle commented Jun 4, 2024

@clpippel friendly reminder if you have time and want to have a look 😸

@maelle
Copy link
Contributor Author

maelle commented Jul 2, 2024

@clpippel friendly reminder 😸

@clpippel
Copy link
Contributor

clpippel commented Jul 2, 2024

I am not sure this is OK. I tried:

library(igraph)
graph <- make_ring(1)
index <- V(graph)
index <- igraph:::as_igraph_vs(graph, index)
(length(index) == vcount(graph) && identical(index, seq_len(vcount(graph))))
# [1] FALSE

The problem is that the class of index is `numeric' and the class of seq_len is 'integer'.
This corrects the problem:

(length(index) == vcount(graph) && identical(index, as.numeric(seq_len(vcount(graph)))))

Assuming vcount(graph) is not double precision.

This was also a surprise to me:

index <- V(g)
identical(index, V(g))
[1] FALSE

@clpippel
Copy link
Contributor

clpippel commented Jul 4, 2024

@maelle, @krlmlr,
Note that E(g) is not identical with itself. Function identical(E(g), E(g)) returns "FALSE". Object E(g) contains a "weak reference", which changes after every call. So it is not a good idea to use identical() together with E(g) or V(g).

It seems consistent to apply the same changes to vertex.attributes() (line 512).

R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
R/attributes.R Outdated Show resolved Hide resolved
@maelle
Copy link
Contributor Author

maelle commented Jul 4, 2024

Thank you @clpippel!!

Yes I have opened #1332 to not forget about the other "confusing" loop. Thanks for the reminder!

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both for looking into it!

R/attributes.R Show resolved Hide resolved
@maelle maelle marked this pull request as ready for review July 4, 2024 11:53
maelle added 2 commits July 4, 2024 13:53
fix: add missing imports
refactor: improve conditions

Apply suggestions from code review

Co-authored-by: Maëlle Salmon <[email protected]>

make it numeric directly
@aviator-app aviator-app bot merged commit 95c335d into main Jul 4, 2024
11 checks passed
@aviator-app aviator-app bot deleted the dup branch July 4, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upkeep maintenance, infrastructure, and similar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor puzzling loop in attributes.R
3 participants