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

Conversation

zigaLuksic
Copy link
Collaborator

Do you think this might be useful?

@zigaLuksic zigaLuksic requested a review from mlubej October 16, 2023 11:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4b185cb) 92.41% compared to head (4b185cb) 92.41%.
Report is 1 commits behind head on develop.

❗ Current head 4b185cb differs from pull request most recent head 9b2608a. Consider uploading reports for the commit 9b2608a to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #760   +/-   ##
========================================
  Coverage    92.41%   92.41%           
========================================
  Files           41       41           
  Lines         4300     4300           
========================================
  Hits          3974     3974           
  Misses         326      326           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eolearn/core/eoexecution.py Outdated Show resolved Hide resolved
@@ -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.

Copy link
Contributor

@mlubej mlubej left a comment

Choose a reason for hiding this comment

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

BTW, perhaps we should be adding to the changelog as we go along, not before each release. This might make our lives easier. We could even add a CI job which fails if the changelog is not updated in case of code updates (but that it's not a requirement, because we don't always need to do it)

@@ -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.

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.

@zigaLuksic
Copy link
Collaborator Author

BTW, perhaps we should be adding to the changelog as we go along, not before each release.

I was thinking about it, but enforcing it seems like a drag compared to 5mins of staring at the history to come up with a condensed history.

I am willing to go with raise_on_temporal_dimension_mismatch though it is long

@mlubej
Copy link
Contributor

mlubej commented Oct 17, 2023

raise_on_temporal_dimension_mismatch

Do you feel that the dimension keyword is necessary? IMO it's clear enough to me, so I would drop it, but if you don't want to drop it, then I'm OK with the current name as well.

@zigaLuksic
Copy link
Collaborator Author

raise_on_temporal_dimension_mismatch

Do you feel that the dimension keyword is necessary? IMO it's clear enough to me, so I would drop it, but if you don't want to drop it, then I'm OK with the current name as well.

I am inclined to keep it because a temporal missmatch is also if the timestamps don't match, but we only check dimensions. But then again the docs do clarify it further 🤔 so maybe we can drop it

@mlubej mlubej self-requested a review October 17, 2023 10:05
@zigaLuksic zigaLuksic merged commit b1f5489 into develop Oct 17, 2023
7 checks passed
@zigaLuksic zigaLuksic deleted the temporal-warnings-as-exceptions branch October 17, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants