Skip to content

chore: restore igraph pointer#799

Merged
Antonov548 merged 5 commits intomainfrom
igraph-r-list
May 16, 2023
Merged

chore: restore igraph pointer#799
Antonov548 merged 5 commits intomainfrom
igraph-r-list

Conversation

@Antonov548
Copy link
Copy Markdown
Contributor

@Antonov548 Antonov548 commented May 13, 2023

Avoid populating igraph_t_idx_oi...igraph_t_idx_is in R

#787

@Antonov548
Copy link
Copy Markdown
Contributor Author

R_igraph_get_pointer restore pointer to igraph from R list structure in case pointer is lost. To restore pointer - R_igraph_restore_pointer is called.

The problem now with old graph structure. Should R_igraph_get_pointer create new environment and restore pointer if R structure hasn't environment,?

@Antonov548 Antonov548 requested a review from krlmlr May 13, 2023 18:08
Copy link
Copy Markdown
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. The code looks good. Why do the checks fail?

@Antonov548
Copy link
Copy Markdown
Contributor Author

Thanks. The code looks good. Why do the checks fail?

As I mentioned in comment, the problem with test check of old data type. In old data type there is no environment, so pointer couldn't be restored in environment. Maybe update_graph function should be updated?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 13, 2023

Missed that. Perhaps R_igraph_restore_pointer() should be split:

  • a function that recreates the native igraph object
  • a function that adds the native igraph object to the environment

The second part would be called only if the environment exists. The problem is that we aren't really allowed to alter the structure of the object in-place.

I'm not sure what the practical consequences are. We could issue a warning in this case, and offer a way to manually upgrade the structure of the graph R object.

Copy link
Copy Markdown
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.

Looks good now, thanks! Please ping me if the next steps are unclear.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented May 16, 2023

Please merge when good.

@krlmlr krlmlr mentioned this pull request May 16, 2023
@Antonov548 Antonov548 marked this pull request as ready for review May 16, 2023 08:14
@Antonov548 Antonov548 merged commit 717a3d7 into main May 16, 2023
@Antonov548 Antonov548 deleted the igraph-r-list branch May 16, 2023 08:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 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.

2 participants