Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible inconsistencies for pulse identifiers when stored using PulseStorage #272

Closed
lumip opened this issue Jun 16, 2018 · 5 comments
Closed

Comments

@lumip
Copy link
Contributor

lumip commented Jun 16, 2018

The dictionary interface of PusleStorage in principle allows it to store pulse under a different identifier than the one they were given on pulse construction, thus creating inconsistencies (or "hidden" renames).

@lumip lumip added this to the New Serialization Routines milestone Jun 16, 2018
@lumip
Copy link
Contributor Author

lumip commented Jul 5, 2018

Resolution: Storing PulseTemplates in PulseStorage under a different identifier should change their identifier in the PulseTemplate as well as in storage. Previous versions stored in PulseStorage under the old identifier should remain unchanged in this case.

@terrorfisch
Copy link
Member

terrorfisch commented Jul 9, 2018

I see three possibilities:

  1. Create a new object with the new identifier (maybe via a new renamed method of Serializable). The problem is that it might be unexpected that a following __getitem__ call gives you a different object.
  2. Throw an error. The user should call that rename method by himself.
  3. Create a Reference like thing. Maybe something like an AbstractPulseTemplate which references the other one on load.

@lumip
Copy link
Contributor Author

lumip commented Jul 9, 2018

The problems with 1 and 3 is that what is actually stored is different from what is input into __setitem__ so a call to __getitem__ would yield a different object which is generally not how the semantics of those methods work, so I actually don't think we should do that.
Option 2 would be the only safe option but would also make the dictionary semantics of PulseStorage obsolete, as they suggest that a user can just pick any identifier to store and PulseTemplate, while in fact, the identifier has to match the one set inside the PT..

Maybe we could consider removing the identifier field of the Serializable class and have the identifier be something that is external to all Serializables and can thus be easily changed by just reassigning the PT to a different id in storage without changing the object itself? This would mean that we couldn't use identifiers directly during sequencing but I don't think that would be a problem (we would just have to use the mapping of id's to PT objects, i.e. the PulseStorage or pulse registry dictionary, during sequencing)

@lumip
Copy link
Contributor Author

lumip commented Jul 12, 2018

This is currently fixed by PR #306 by introducing a method Serializable.renamed(new_identifier) which returns a copy of the original Serializable object with the given new identifier to enable explicit renames and having PulseStorage raise an error (which points to the possibility of renamed()) when attempting to store a PulseTemplate/Serializable under a different identifier than its internal one.

@terrorfisch
Copy link
Member

Fixed as we throw an exception now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants