-
Notifications
You must be signed in to change notification settings - Fork 27
Clarify Pipeline __getitem__ and apply step handling #268
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,15 +553,52 @@ def __getitem__(self, idx: Any): | |
|
|
||
| # Apply each pipeline step to the sample, starting from the latest source | ||
| for step in self.steps[step_index + 1 :]: | ||
| if not isinstance(step, (Pipeline, PipelineStep, Transform, TransformCollection)): | ||
| raise TypeError(f"When iterating through pipeline steps, found a {type(step)} which cannot be parsed.") | ||
| if isinstance(step, PipelineIndex): | ||
| # A PipelineIndex appearing after the data-source position means the | ||
| # steps list contains two PipelineIndex objects (e.g. two TemporalWindow | ||
| # instances). Only the last one (found by _get_initial_sample's reverse | ||
| # search) acts as the source; any earlier one would need an idx to | ||
| # re-retrieve data, which __getitem__ does not support at this point in | ||
| # the execution path. Raise a clear error rather than silently | ||
| # mis-applying the step. | ||
| raise TypeError( | ||
| f"Pipeline.__getitem__ encountered a {type(step).__name__} " | ||
| f"({type(step)}) after the data-source position in the step list. " | ||
| "Only one PipelineIndex per pipeline is supported: it must be the " | ||
| "last step that acts as the data source (found by the reverse search " | ||
| "in _get_initial_sample). " | ||
| "If you need to chain two PipelineIndex objects, nest the first " | ||
| "pipeline (containing the accessor and first PipelineIndex) inside " | ||
| "a second Pipeline, then attach the second PipelineIndex to that." | ||
| ) | ||
| if not isinstance( | ||
| step, | ||
| ( | ||
| Pipeline, | ||
| PipelineStep, | ||
| Operation, | ||
| Transform, | ||
| TransformCollection, | ||
| pyearthtools.pipeline.branching.PipelineBranchPoint, | ||
| ), | ||
| ): | ||
| raise TypeError( | ||
| f"Pipeline.__getitem__ encountered a {type(step)} in the step list " | ||
| "after the data-source position, which cannot be applied to a sample. " | ||
| "Steps after the data source must be PipelineStep, Operation, " | ||
| "Transform, TransformCollection, Pipeline, or PipelineBranchPoint." | ||
| ) | ||
| LOG.debug(f"Apply step upon sample: {step.__class__.__qualname__}") | ||
|
|
||
| if isinstance(step, Pipeline): | ||
| sample = step.apply(sample) | ||
| elif isinstance(step, pyearthtools.pipeline.branching.PipelineBranchPoint): | ||
| with pyearthtools.utils.context.ChangeValue(step, "_current_idx", idx): | ||
| sample = step.apply(sample) | ||
| elif isinstance(step, PipelineStep): | ||
| sample = step.run(sample) | ||
| elif isinstance(step, Operation): | ||
| sample = step.apply(sample) | ||
| else: | ||
| sample = step(sample) # type: ignore | ||
|
|
||
|
|
@@ -579,11 +616,32 @@ def get(self, idx): | |
|
|
||
| def apply(self, sample): | ||
| """ | ||
| Apply pipeline to `sample` | ||
|
|
||
| `Pipeline` should only consist of `PipelineStep`'s and `Transforms`, as `Indexes` cannot be applied, | ||
| Apply pipeline to `sample`. | ||
|
|
||
| Runs each step in the pipeline against an already-retrieved data sample. | ||
| Steps must be one of: PipelineStep, Operation, Pipeline, Transform, | ||
| TransformCollection, or PipelineBranchPoint. | ||
|
|
||
| PipelineIndex steps (e.g. TemporalWindow, SequenceRetrieval) cannot be | ||
| applied via this method because they operate on an *index* (such as a date | ||
| string) to retrieve data, not on an existing sample. If your pipeline | ||
| contains a PipelineIndex, use ``pipeline[idx]`` instead of | ||
| ``pipeline.apply(sample)``. PipelineIndex steps are handled automatically | ||
| by ``__getitem__`` via ``_get_initial_sample``'s reverse search. | ||
| """ | ||
| for step in self.steps: | ||
| if isinstance(step, PipelineIndex): | ||
| raise TypeError( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about this, particularly in the context of a caching step, maybe the index class in question should have a flag to indicate if skipping is safe. Skipping caching is still valid mathematically, so you could either skip it silents or raise a warning rather than an exception. However, other things like a temporal window aren't valid, because they need to reach back upstream in the pipeline and retrieve additional data, and perhaps it's appropriate to raise the exception in those circumstances. What do you think about that idea? |
||
| f"Pipeline.apply() encountered a {type(step).__name__} " | ||
| f"({type(step)}) which cannot be applied to an existing sample. " | ||
| f"{type(step).__name__} is a PipelineIndex: it retrieves data from " | ||
| "an index (e.g. a date string) and has no meaningful behaviour when " | ||
| "called on an already-retrieved sample. " | ||
| "Use pipeline[idx] instead of pipeline.apply(sample) when your " | ||
| "pipeline contains a PipelineIndex step such as TemporalWindow or " | ||
| "SequenceRetrieval. pipeline[idx] handles PipelineIndex steps " | ||
| "automatically via _get_initial_sample." | ||
| ) | ||
| if not isinstance( | ||
| step, | ||
| ( | ||
|
|
@@ -595,7 +653,12 @@ def apply(self, sample): | |
| pyearthtools.pipeline.branching.PipelineBranchPoint, | ||
| ), | ||
| ): | ||
| raise TypeError(f"When iterating through pipeline steps, found a {type(step)} which cannot be parsed.") | ||
| raise TypeError( | ||
| f"Pipeline.apply() encountered a {type(step)} in the step list " | ||
| "which cannot be applied to a sample. " | ||
| "Steps must be PipelineStep, Operation, Transform, " | ||
| "TransformCollection, Pipeline, or PipelineBranchPoint." | ||
| ) | ||
| if isinstance(step, Pipeline): | ||
| sample = step.apply(sample) # type: ignore | ||
| elif isinstance(step, PipelineStep): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to dive into this to understand the example fully. A cache object is an index which can be included after the initial data source index, so that circumstance needs to be accounted for. The last one (e.g. the cache or temporal window) is supposed to use the parent pipeline to retrieve the individual samples, since the data source index might not be the same as the cache/subsequent index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, reading ahead I've just understood the situation more clearly. The use case is attempting to use the pipeline apply method on a single already-retrieved sample, and the index steps are interfering with its execution in an unexpected way.