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

Update identification of BaseFormat during benchmark.yml run #116

Closed
wants to merge 1 commit into from

Conversation

briangow
Copy link
Collaborator

When a new / updated format is pushed to the repo we run a test from benchmark.yml. This test intends to exclude the base formats from the test by searching for Base at the start of the class name. However, not all of the base formats start with Base. All of the base formats are subclasses of BaseFormat though.

This PR searches for BaseFormat in the object bases instead of a class name starting with Base.

@briangow briangow requested a review from tompollard November 15, 2024 20:57
@tompollard
Copy link
Contributor

In the guidelines at: https://github.com/chorus-ai/chorus_waveform/blob/main/BENCHMARK.md we say:

If you are defining additional base classes that are not intended to be benchmarked directly, you should include Base at the start of the name (for example, "BaseWFDBFormat"). This allows us to skip the class in the benchmark workflow.

If we are following this approach, then ideally any base formats (i.e. formats that we do not expect to be run directly) should include Base at the start of the name.

The problem with excluding all classes that inherit from BaseFormat is that this will exclude some classes that should be run. e.g.: https://github.com/chorus-ai/chorus_waveform/blob/main/waveform_benchmark/formats/vital.py

@briangow
Copy link
Collaborator Author

Ok, fair enough, I'll close this and update the formats that are missing "Base" at the start of a base class that shouldn't be run.

@tompollard
Copy link
Contributor

An alternative is that we add an explicit flag that determines whether or not the benchmark tests should be run.

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.

2 participants