DM-26371 Update code to support new QuantumGraph type #71
Conversation
natelust
force-pushed
the
tickets/DM-26371
branch
from
608d5ef
to
9ebe636
Compare
| """ | ||
|
|
||
| def __init__(self, taskLabel: str, dimensions: Union[str, Sequence[str]], reverse: bool = False): | ||
| self.taskLabel = taskLabel | ||
| self.dimensions = dimensions | ||
| self.reverse = reverse |
There was a problem hiding this comment.
You did not remove it from parameter list? Why reverse is not supported, is it impossible in a new world?
| for qdata in keyQuanta[key]: | ||
| qdata.dependencies |= prev_indices | ||
| return quanta | ||
| quanta = list(graph.quantaForTask(taskDef)) |
There was a problem hiding this comment.
What order do they come out? Previous version ordered them by dataId, can't we do the same here?
| @@ -54,14 +56,16 @@ class _Job: | |||
|
|
|||
| Parameters | |||
| ---------- | |||
| qdata : `~lsst.pipe.base.QuantumIterData` | |||
| qdata : `~lsst.pipe.base.QuantumNode` | |||
There was a problem hiding this comment.
I would rename qdata to qnode.
| @@ -54,14 +56,16 @@ class _Job: | |||
|
|
|||
| Parameters | |||
| ---------- | |||
| qdata : `~lsst.pipe.base.QuantumIterData` | |||
| qdata : `~lsst.pipe.base.QuantumNode` | |||
| Quantum and some associated information. | |||
| """ | |||
There was a problem hiding this comment.
CAn you add docstring for two new arguments.
There was a problem hiding this comment.
They were not needed any more and were removed instead
| @@ -164,7 +168,7 @@ def finishedIds(self): | |||
| jobsIds : `set` [`int`] | |||
| Set of integer job IDs. | |||
| """ | |||
| return set(job.qdata.index for job in self.jobs if job.state == JobState.FINISHED) | |||
| return set(job.qdata for job in self.jobs if job.state == JobState.FINISHED) | |||
There was a problem hiding this comment.
Should it be job.index instead? Same for two other changes below. If it needs to return nodes instead of index then it's better to rename methods and update docstrings.
| quantaIter : iterable of `~lsst.pipe.base.QuantumIterData` | ||
| Possibly updated set of quanta, properly ordered for execution. | ||
| graph : `QuantumGraph` | ||
| Modified `QuantumGraph`, properly ordered for execution. |
There was a problem hiding this comment.
I think new QuantumGRaph cannot be non-ordered, may be drop this "properly ordered..."
| @@ -181,22 +218,18 @@ def test_mpexec_fixup(self): | |||
|
|
|||
| taskDef = TaskDefMock() | |||
|
|
|||
| for reverse in (False, True): | |||
There was a problem hiding this comment.
Would be nice if we could keep reverse.
|
|
||
| Returns | ||
| ------- | ||
| update : `~lsst.daf.butler.Quantum` |
There was a problem hiding this comment.
Can you call it quantum instead of update? Or updatedQuantum?
natelust
force-pushed
the
tickets/DM-26371
branch
from
467e868
to
c64102a
Compare
natelust
force-pushed
the
tickets/DM-26371
branch
from
c64102a
to
a383768
Compare
No description provided.