Update virtual_node.py

[mzamini92]

Instead of creating new tensors in each iteration, can't we modify the existing tensors in-place? also, instead of creating new tensors by concatenating existing tensors, isn't it better we use tensor views to represent the augmented tensors? It seems in old_data = copy.copy(data) the subsequent operations only modify the data object and don't change the underlying tensor data. are these valid points?

[codecov]

Codecov Report

Merging #7751 (fd53e6c) into master (c6f3a55) will decrease coverage by 0.36%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #7751      +/-   ##
==========================================
- Coverage   91.91%   91.56%   -0.36%     
==========================================
  Files         452      452              
  Lines       25550    25543       -7     
==========================================
- Hits        23485    23389      -96     
- Misses       2065     2154      +89     

Impacted Files	Coverage Δ
torch_geometric/transforms/virtual_node.py	95.23% <91.66%> (-0.42%)	⬇️

... and 19 files with indirect coverage changes

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

[akihironitta]

I guess the idea behind making a copy is that we want to avoid silently contaminating the original data. I believe that's the motivation for #7429.

However, for your change in this PR specifically, the data is already copy.copy()'ed in the BaseTransform.__call__, so it shouldn't change any behaviour.

[mzamini92]

I guess the idea behind making a copy is that we want to avoid silently contaminating the original data. I believe that's the motivation for #7429.

However, for your change in this PR specifically, the data is already copy.copy()'ed in the BaseTransform.__call__, so it shouldn't change any behaviour.

Thank you. if that's the case then yes maybe having copy.copy() is not bad. do we need to close the PR? or I should do anything special to merge it?

[rusty1s]

We need the separation here between old_data and data since we modify data in the for-loop (and potentially its number of nodes/edges), and thus is_node_attr and is_edge_attr inside the loop may fail to recognize node-level and edge-level tensors since their size does no longer match.

Feel free to re-open if you have strong concerns.