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

[Refactor Suggestions I] Mixins all the way #19

Closed
wants to merge 1 commit into from

Conversation

CodyCBakerPhD
Copy link
Collaborator

First of several draft proposals opened for comparison; each offers advantages and disadvantages in terms of readability and comprehension (dependent on knowledge of package structure)

This one pushes the mixins to the ultimate level, so that each actual benchmark run by ASV is only comprised of the specification of that particular instances attributes

The changelog view is a little muddled but the direct file view showcases how easy it is to read: https://github.com/NeurodataWithoutBorders/nwb_benchmarks/blob/1f58d3830e53f5177be97088355f05a59e1fdf63/src/nwb_benchmarks/benchmarks/streaming.py

However, this approach comes with a lot of implicit understanding of how the mixins are separated, with a lot of redirection required to understand what any single benchmark is actually doing whenever it runs

But it does make it really easy to expand a matrix of cases by taking cross-combinations of setups and time tests

Also beginning to adopt sklearn import structure since centralized imports are now being used

@CodyCBakerPhD
Copy link
Collaborator Author

The implicit knowledge of how to use this PR is the most of any of them, but the pattern can be learned and documented

Each benchmark is a mixin of the structure ChildBenchmark(BaseBenchmark, BaseSetup, ExtraMethodsUsedByEitherBase)

where BaseBenchmark defines all the tracking tests to run, and BaseSetup defines the setup and teardown methods to occur around those tracking tests; both of these might rely on interchangeable intermediate methods that are specified by the extra mixins

The resulting benchmark file is very easy to read, but takes a lot of redirection (one per mixin) to understand exactly what is is running

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that the main benchmarks folder is for things ASV will actually point to; base class definitions should probably be outside of this

Analogous to having a src/testing submodule that lives outside of an outer tests folder when using pytest

@CodyCBakerPhD CodyCBakerPhD deleted the refactor_extreme_mixins branch February 21, 2024 20:54
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.

1 participant