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

Add option to treat temporal warnings as errors in eoexecutions #760

Merged
merged 5 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- `MorphologicalFilterTask` adapted to work on boolean values.
- Added `temporal_subset` method to `EOPatch`, which can be used to extract a subset of an `EOPatch` by filtering out temporal slices. Also added a corresponding `TemporalSubsetTask`.
- `EOExecutor` now has an option to treat `TemporalDimensionWarning` as an exception.

## [Version 1.5.0] - 2023-09-06

Expand Down
12 changes: 10 additions & 2 deletions eolearn/core/eoexecution.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

from .eonode import EONode
from .eoworkflow import EOWorkflow, WorkflowResults
from .exceptions import EORuntimeWarning
from .exceptions import EORuntimeWarning, TemporalDimensionWarning
from .utils.fs import get_base_filesystem_and_path, get_full_path, pickle_fs, unpickle_fs
from .utils.logging import LogFileFilter
from .utils.parallelize import _decide_processing_type, _ProcessingType, parallelize
Expand Down Expand Up @@ -55,6 +55,7 @@ class _ProcessingData:
filter_logs_by_thread: bool
logs_filter: Filter | None
logs_handler_factory: _HandlerFactoryType
temporal_dim_warning_is_error: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having doubts about the name, but not sure about alternatives. Maybe:

  • raise_on_temporal_mismatch
  • temporal_mismatch_is_error

or something along those lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, no matter what you try you end up with something long 🙈 I was thinking of shortening it to raise_on_tdim_missmatch but tdim is kinda non-standard

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but also, this one is for a private object. Is the name of the parameter in the function ok or does that bother you as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, generally I meant more for the parameter in the function than the private one. Perhaps I'm more inclined to use the one with raise because I remember seeing it in libraries a few times as a parameter as well.

but in the end it's a specific thing, which requires a specific name, so if you don't like the suggestions, it's fine and feel free to leave it as it is.



@dataclass(frozen=True)
Expand Down Expand Up @@ -86,6 +87,7 @@ def __init__(
filesystem: FS | None = None,
logs_filter: Filter | None = None,
logs_handler_factory: _HandlerFactoryType = FileHandler,
temporal_dimension_warning_is_error: bool = False,
mlubej marked this conversation as resolved.
Show resolved Hide resolved
):
"""
:param workflow: A prepared instance of EOWorkflow class
Expand All @@ -108,6 +110,7 @@ def __init__(
object.

The 2nd option is chosen only if `filesystem` parameter exists in the signature.
:param temporal_dimension_warning_is_error: Whether to treat `TemporalDimensionWarning` as an exception.
"""
self.workflow = workflow
self.execution_kwargs = self._parse_and_validate_execution_kwargs(execution_kwargs)
Expand All @@ -116,6 +119,7 @@ def __init__(
self.filesystem, self.logs_folder = self._parse_logs_filesystem(filesystem, logs_folder)
self.logs_filter = logs_filter
self.logs_handler_factory = logs_handler_factory
self.temporal_dim_warning_is_error = temporal_dimension_warning_is_error

self.start_time: dt.datetime | None = None
self.report_folder: str | None = None
Expand Down Expand Up @@ -193,6 +197,7 @@ def run(self, workers: int | None = 1, multiprocess: bool = True, **tqdm_kwargs:
filter_logs_by_thread=filter_logs_by_thread,
logs_filter=self.logs_filter,
logs_handler_factory=self.logs_handler_factory,
temporal_dim_warning_is_error=self.temporal_dim_warning_is_error,
)
for workflow_kwargs, log_path in zip(self.execution_kwargs, log_paths)
]
Expand Down Expand Up @@ -263,7 +268,10 @@ def _execute_workflow(cls, data: _ProcessingData) -> WorkflowResults:
data.logs_handler_factory,
)

results = data.workflow.execute(data.workflow_kwargs, raise_errors=False)
with warnings.catch_warnings():
if data.temporal_dim_warning_is_error:
warnings.simplefilter("error", TemporalDimensionWarning)
results = data.workflow.execute(data.workflow_kwargs, raise_errors=False)

cls._try_remove_logging(data.log_path, logger, handler)
return results
Expand Down
34 changes: 33 additions & 1 deletion tests/core/test_eoexecutor.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,21 @@
import pytest
from fs.base import FS

from eolearn.core import EOExecutor, EONode, EOTask, EOWorkflow, OutputTask, WorkflowResults, execute_with_mp_lock
from sentinelhub import CRS, BBox

from eolearn.core import (
CreateEOPatchTask,
EOExecutor,
EONode,
EOTask,
EOWorkflow,
FeatureType,
InitializeFeatureTask,
OutputTask,
WorkflowResults,
execute_with_mp_lock,
linearly_connect_tasks,
)
from eolearn.core.utils.fs import get_full_path

FULL_LOG_LINE_COUNT = 12
Expand Down Expand Up @@ -251,3 +265,21 @@ def test_without_lock(num_workers):
assert len(lines) == 2 * num_workers
assert len(set(lines[:num_workers])) == num_workers, "All processes should start"
assert len(set(lines[num_workers:])) == num_workers, "All processes should finish"


@pytest.mark.parametrize("multiprocess", [True, False])
def test_temporal_dim_error(multiprocess):
workflow = EOWorkflow(
linearly_connect_tasks(
CreateEOPatchTask(bbox=BBox((0, 0, 1, 1), CRS.POP_WEB)),
InitializeFeatureTask([FeatureType.DATA, "data"], (2, 5, 5, 1)),
)
)

executor = EOExecutor(workflow, [{}, {}])
for result in executor.run(workers=2, multiprocess=multiprocess):
assert result.error_node_uid is None

executor = EOExecutor(workflow, [{}, {}], temporal_dimension_warning_is_error=True)
mlubej marked this conversation as resolved.
Show resolved Hide resolved
for result in executor.run(workers=2, multiprocess=multiprocess):
assert result.error_node_uid is not None