Conversation
github-actions
Bot
changed the title
| parts["data"] = wavs | ||
| result = da.bins.assign_coords(wavelength=sc.bins(**parts, validate_indices=False)) | ||
| out = result.bins.drop_coords("event_time_offset") | ||
| out = da.bins.assign_coords(wavelength=sc.bins(**parts, validate_indices=False)) |
There was a problem hiding this comment.
Keeping an extra coord cost both memory and compute for all instruments and follow up operations. I don't think we can do this (not drop the coord) as a generally enabled thing.
There was a problem hiding this comment.
I understand what you're saying, but consider that the coord could be dropped in the instrument package code and then it would not cost extra compute or memory.
It would use extra memory in-between this drop and the drop in the package code, but that will likely be a short period between those run, and max memory usage is unlikely to increase.
But if we don't make this change, what alternative do you prefer?
There was a problem hiding this comment.
Isn't it already costing in the very same function a couple lines further down?
How about adding a workflow param that can optionally enable keeping this coord?
There was a problem hiding this comment.
Isn't it already costing in the very same function a couple lines further down?
I don't think so. What code section do you mean?
How about adding a workflow param that can optionally enable keeping this coord?
I'm fine with that too.
There was a problem hiding this comment.
I mean the if "event_time_zero" in out.dims: block.
There was a problem hiding this comment.
Aha I see. 👍 I thought that less important because it only runs for event monitors and not for regular detectors. But I guess it could be relevant for larger event monitors.
SimonHeybrock
changed the title
event_time_offset from WavelengthData
| run_types: Iterable[sciline.typing.Key], | ||
| monitor_types: Iterable[sciline.typing.Key], | ||
| wavelength_from: WavelengthLutMode = "file", | ||
| keep_event_time_offset: bool = False, |
There was a problem hiding this comment.
Why did you choose to make this a workflow-factory param? It does not affect workflow structure (likely the other args above), so we can keep it a plain Pipeline param, right?
There was a problem hiding this comment.
Yes that's much better 👍 Fixed that and some other structure issues.
2 participants
Fixes #593
I know there were some disagreement on exactly what we should do here.
But it is at least easier to have a PR with a suggested implementation to base the discussion on.