Skip to content

Conversation

@jl-wynen
Copy link
Member

@jl-wynen jl-wynen commented Nov 3, 2025

No description provided.

@jl-wynen jl-wynen requested a review from nvaytet November 3, 2025 14:19
@jl-wynen jl-wynen moved this to Selected in Development Board Nov 3, 2025
@nvaytet nvaytet self-assigned this Nov 4, 2025
"outputs": [],
"source": [
"masked = wf.compute(CountsMasked[SampleRun])\n",
"masked = wf.compute(MaskedDetector[SampleRun])\n",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be CorrectedDetector instead of MaskedDetector?

MonitorType = TypeVar(
"MonitorType", BeamMonitor1, BeamMonitor2, BeamMonitor3, BeamMonitor4
)
RunType = reduce_t.RunType
Copy link
Member

Choose a reason for hiding this comment

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

We would like to keep the imaging runtypes here, as the names fit better than the ones in essreduce.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? The names haven't changed. The only difference is that the type var is no longer constrained. This has do be done when constructing a pipeline.

Mixing type vars is tricky. If they happen to get different constraints (ess.reduce.nexus.types.RunType via Pipeline(constraints=...) and ess.imaging.types.RunType via TypeVar contraints), then the pipeline will fail to build with obscure errors. I changed this definition precisely because I encountered such an error.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I misunderstood, I thought we now had to use the names from essreduce.
Then it's fine 👍

@jl-wynen jl-wynen merged commit b56f04f into main Nov 4, 2025
4 checks passed
@jl-wynen jl-wynen deleted the standard-domain-types branch November 4, 2025 09:00
@github-project-automation github-project-automation bot moved this from Selected to Done in Development Board Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants