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

adding duplicate data filter #30

Merged
merged 3 commits into from
May 27, 2024
Merged

adding duplicate data filter #30

merged 3 commits into from
May 27, 2024

Conversation

heikoklein
Copy link
Member

@heikoklein heikoklein commented May 23, 2024

This PR adds a duplicate filter to pyaro. By default, it filters duplicates of type "stations", "start_times", "end_times", but the user can configure other keys.

This PR starts also using typing-features not available in python 3.9. Following pyaerocom, python 3.9 is therefore no longer supported, and python 3.12 has been added to the list of supported python versions.

closes #22

@heikoklein heikoklein added this to the m2024-06 milestone May 23, 2024
@heikoklein heikoklein requested a review from avaldebe May 23, 2024 15:43
@heikoklein heikoklein linked an issue May 23, 2024 that may be closed by this pull request
Copy link
Collaborator

@avaldebe avaldebe left a comment

Choose a reason for hiding this comment

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

I see a problem on the AutoFilterEngine.supported_filters implementation, which is not part of this PR.

        return self.reader_class().supported_filters()

reader_class returns a generic Reader, which does not require a supported_filters method.

I think that reader_class should returns a AutoFilterReader.

I also see many looks filling up lists across the module, that are cleaner as list comprehensions.
Also, UnkownFilterException is misspell.

src/pyaro/timeseries/Filter.py Show resolved Hide resolved
src/pyaro/timeseries/AutoFilterReaderEngine.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@heikoklein
Copy link
Member Author

@avaldebe Thanks for the excellent review. I added nearly all of your suggestions, and concerning typing fixed similar errors in other parts of the code.
I'm not sure if reader_class should use a type-hint returning the abstract base Reader or the implementation AutoFilterReader. The programmer using this code should only work with interface of the basic Reader anyway and not with the specifics of the AutoFilterReader. In python 3.11, this might have been nicer by return -> Self, but for 3.10, I keep the base-class.

@heikoklein heikoklein requested a review from avaldebe May 24, 2024 13:00
Copy link
Collaborator

@avaldebe avaldebe left a comment

Choose a reason for hiding this comment

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

thanks for addressed the issues I raised

@heikoklein heikoklein merged commit 628f3bd into main May 27, 2024
1 check passed
@heikoklein heikoklein deleted the 22-duplicate-filter branch May 27, 2024 07:57
@heikoklein
Copy link
Member Author

Looking again at the AutoFilterEngine base-class and your first comment, I agree with you and return now a AutoFilterReader instead of just a Reader. Sorry for misunderstanding when reading the first time, I got confused since the comment was not related to this PR. I will fix that in #32

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.

Duplicate Filter
2 participants