Skip to content

Comments

Special case compute_identity edges to forward ref types#5302

Merged
ffreyer merged 5 commits intomasterfrom
ff/forward_node_ref_type
Sep 25, 2025
Merged

Special case compute_identity edges to forward ref types#5302
ffreyer merged 5 commits intomasterfrom
ff/forward_node_ref_type

Conversation

@ffreyer
Copy link
Collaborator

@ffreyer ffreyer commented Sep 16, 2025

Description

When passing a node of graph A as an input to graph B a new node is created in graph B which connects to the node in A with compute_identity. This was done to avoid sharing nodes between multiple graphs so that nodes are easier to manage. Since this was just using the normal machinery, the new node would pin whatever type it first gets.

This pr changes that behavior by special casing compute_identity. It now reuses the Refs in input nodes directly, which preserves their potentially abstract eltypes and also allows us to skip a bunch of update code. So maybe this is also slightly faster.

This is in relation to JuliaDataCubes/PyramidScheme.jl#78.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@github-project-automation github-project-automation bot moved this to Work in progress in PR review Sep 16, 2025
@MakieBot
Copy link
Collaborator

MakieBot commented Sep 16, 2025

Benchmark Results

SHA: bd67d09aa162fe2b3b1b4df191afe3e3fdd54f06

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

Copy link
Member

@SimonDanisch SimonDanisch left a comment

Choose a reason for hiding this comment

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

Seems like a good idea and ready to merge if tests pass.

@ffreyer ffreyer marked this pull request as ready for review September 25, 2025 00:10
@ffreyer ffreyer merged commit 42df6c7 into master Sep 25, 2025
27 checks passed
@ffreyer ffreyer deleted the ff/forward_node_ref_type branch September 25, 2025 00:10
@github-project-automation github-project-automation bot moved this from Work in progress to Merged in PR review Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

3 participants