Fix logic for handling repetition_ids in CircuitOperation.replace

[maffoo]

Previously, passing repetition_ids to replace would enable use_repetition_ids, even if the value passed was None. This fixes things so we only enable use_repetition_ids if a non-None value is passed, which is the same as what is done in the CircuitOperation constructor.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.89%. Comparing base (116cf6e) to head (73837a0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6984   +/-   ##
=======================================
  Coverage   97.89%   97.89%           
=======================================
  Files        1085     1085           
  Lines       95034    95050   +16     
=======================================
+ Hits        93032    93048   +16     
  Misses       2002     2002           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[daxfohl]

Seems like the only use case of use_repetition_ids now is for users that do CircuitOperation(..., repetitions=n, use_repetition_ids=True). But they'd be able to do CircuitOperation(..., repetition_ids=list(range(n))) in fewer keystrokes if that's what they wanted.

When repetition ids were the default behavior, this flag was necessary to allow it to be turned off. But now that it's not the default behavior, should the field be deprecated entirely? (Or removed without deprecation, given the behavior modification was already a breaking change).

[maffoo]

Seems like the only use case of use_repetition_ids now is for users that do CircuitOperation(..., repetitions=n, use_repetition_ids=True). But they'd be able to do CircuitOperation(..., repetition_ids=list(range(n))) in fewer keystrokes if that's what they wanted.

repetition_ids=list(range(n)) doesn't actually work because repetition ids are supposed to be strings. So you'd have to do something like repetition_ids=[str(i) for i in range(n)]. You're right that the only point of having use_repetition_ids is to avoid having to manually create these "default" repetition ids, though. Another possibility would be to use a different sentinel for the default, for example True. Then you could pass repetition_ids=True to enable using the default ids for the given number of repetitions.

When repetition ids were the default behavior, this flag was necessary to allow it to be turned off. But now that it's not the default behavior, should the field be deprecated entirely? (Or removed without deprecation, given the behavior modification was already a breaking change).

I would be fine with removing the use_repetition_ids field, though I would prefer to fix the broken logic for now and do that in a separate change where we can discuss things like the default sentinel mentioned above.

[daxfohl]

Lgtm

[mhucka]

LGTM