`remove_duplicates()` without `in_place = True` causes `signature_value` to be not updated after `write`.

[btasdelen]

Due to the copy of Sequence object being made when in_place=False, assignments to the self in write() function is done to the copy, not the original. As a result, those values are not assigned after the call returns. AFAIK, only signature only these three lines are effected:

https://github.com/imr-framework/pypulseq/blob/10820cd22921eed477e0df7569949c9b8499f7e9/pypulseq/Sequence/write_seq.py#L261-L263

As a dummy workaround, I just passed in_place=True to the remove_duplicates(), but I think that is not what we want, as it will modify the Sequence object, is that true? In that case, my suggestion is to rename the copy, instead of assigning it to self, and still assigning signatures to self to keep old behavior. I can make that commit if you can confirm this is the desired behavior.

[FrankZijlstra]

Specifying in_place=True will end up rounding values in the sequence libraries, which I think is undesirable. Renaming the copy is probably the desired behaviour, though it will be storing the hash for the sequence with duplicate events removed (i.e. not exactly the same object as where the values would then be stored). Perhaps a related question, what is the purpose of those signature values being stored in the object? In both pypulseq and pulseq I only see it ever being written, never accessed. It might make more sense to have it as a return value for the write function?

[btasdelen]

In our workflow, we use the signature to name a metadata file so that correct metadata can be automatically picked and used in the reconstruction.

Returning signature also works. Behavior change bothers me a bit, but I don't think many people are using signatures, it should be ok. I will make the change and commit.

[btasdelen]

@FrankZijlstra I made the changes, feel free to merge if everything looks good.

[aTrotier]

I'm using the signature to assert if the trajectory from .seq used during reconstruction is the same as the one in the metadata of the converted .mrd

Le jeu. 13 juin 2024, 18:06, Bilal Taşdelen @.***> a écrit :

@FrankZijlstra https://github.com/FrankZijlstra I made the changes, feel free to merge if everything looks good.

— Reply to this email directly, view it on GitHub https://github.com/imr-framework/pypulseq/pull/179#issuecomment-2166111203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5P7O46LDH3P55VG2J4BOTZHG7QNAVCNFSM6AAAAABJHK5KFCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRWGEYTCMRQGM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[btasdelen]

On a second thought, considering @aTrotier and our use cases and potential uses, it makes sense that one does not need the Sequence object's signature, but the written file's signature anyway. To keep backward compatibility and not break anyone's code, I think it is best to assign the signature to the original class, and also return from the write function.

However, I am not a fan of write function having a side effect as it is not a class method, so I will add the assignment to the Sequence.write() function. I will document this behavior.