ManiSkill icon indicating copy to clipboard operation
ManiSkill copied to clipboard

Implicit copy in Pose.create

Open BlGene opened this issue 1 year ago • 2 comments

Hi Team,

this just caused a bug for me and feels a bit inconsistent. My expectation would be that Pose.create creates a new instance, ChatGPT thinks so too ;)

(Pdb) new_pose = Pose.create(start_pose)
(Pdb) id(new_pose)
132378980628320
(Pdb) id(start_pose)
132378980628320
(Pdb) new_pose = Pose.create_from_pq(p=start_pose.get_p(), q=start_pose.get_q())
(Pdb) id(new_pose)
132378980629664

(Pdb) mani_skill.__version__
'3.0.0b10'

BlGene avatar Oct 28 '24 10:10 BlGene

I would think so as well lol. Thanks for bringing this up. https://github.com/haosulab/ManiSkill/blob/3086a0fc8fa5523b10bb95e8fc3b5336593c54dc/mani_skill/utils/structs/pose.py#L125

Is the culprit probably. Would also need to clone the original raw pose data. I think the one where you do new_pose = Pose.create_from_pq(p=start_pose.get_p(), q=start_pose.get_q()) might also be problematic since if start_pose changes new_pose might also change.

StoneT2000 avatar Oct 28 '24 18:10 StoneT2000

Yes, I ended up making it do what I want with pos_new = start_pose.get_p().clone().detach() ... new_pose.set_p(pos_new)

I don't know your API well, but it looks to me like these might be reasonable options:

  1. new_pose = start_pose.clone().detach() (the torch way)
  2. new_pose = Pose.create(start_pose) (the numpy way, also works with torch, but not recommended)

BlGene avatar Oct 29 '24 08:10 BlGene

closed by #688

Opted to not clone directly since there could be useful optimizations when storing a very large matrix of poses that you might not want to clone and use up memory / time. If you really want to clone it then just clone the raw pose yourself.

StoneT2000 avatar Nov 07 '24 21:11 StoneT2000