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

DM-29699: CalibCombineConnections changes its quantum dimensions at construction #85

Merged
merged 5 commits into from May 6, 2021

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Apr 21, 2021

Use separate classes for the cpCombine step when it does and does not use the 'physical_filter' dimension.

The new CalibCombineByFilter Connections, Config, and Task now inherit
from their respective CalibCombine counterparts, but simply add the
new dimension. This avoids the previous work around to have the
dimensions edited on construction.

Update the pipeline definitions for this as well.

Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good simplification. Two questions. (1) It's fine to have CalibCombineByFilterTask be the default, but what would the vanilla CalibCombineTask ever be used for? (2) What would happen if someone did try to run a pipeline that still explicitly gave calibrationDimensions, and can a useful error be raised?

@czwa
Copy link
Contributor Author

czwa commented Apr 30, 2021

CalibCombineTask is still used for bias and dark creation, as those should have no illumination at all (and so can ignore filters). If an prior pipeline were run with the new processing code, there'd be a dimension mismatch and an error. If I'm thinking about it correctly, that error would be raised while constructing the pipeline quantum graph, and so adding a message in the cp_pipe code wouldn't help.

Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bracing myself for people to ask about qgraph dimension errors in a month or so when their stack versions catch up with this change 😁 Looks great otherwise!

use the 'physical_filter' dimension.

The new CalibCombineByFilter Connections, Config, and Task now inherit
from their respective CalibCombine counterparts, but simply add the
new dimension.  This avoids the previous work around to have the
dimensions edited on construction.

Update the pipeline definitions for this as well.
@czwa czwa merged commit d45243f into master May 6, 2021
@czwa czwa deleted the tickets/DM-29699 branch May 6, 2021 22:28
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

Successfully merging this pull request may close these issues.

None yet

2 participants