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 II] Functional, with some inheritance #20

Closed
wants to merge 2 commits 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 centralizes duplicate code into common functions, including some base benchmarks where appropriate

Reduces redirection compared to #19, but still requires some understanding of the base benchmark definitions to understand what tests are being run in the cases where a benchmark inherits

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

Comment on lines +1 to +8
class AcquisitionTimeSeriesSliceBenchmark:
s3_url: str
acquisition_path: str
slice_range: tuple # TODO: enhance annotation

def time_slice(self):
# Store as self._temp to avoid tracking garbage collection as well
self._temp = self.nwbfile.acquisition[self.acquisition_path].data[self.slice_range]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a couple of things I noticed while working on the network benchmarks:

  1. When using setup_cache, ASV appear to run the cache setup only once for the base class, but does not run it again for child classes that inherit from that class.
  2. We wouldn't want to run time_slice in the base class, I assume adding the underscore prefix in the py filename is to prevent ASV from detecting the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. When using setup_cache, ASV appear to run the cache setup only once for the base class, but does not run it again for child classes that inherit from that class.

Great to know; I'd consider this a point in favor of #21 since it removes the need for having to know that implicit behavior of ASV

  1. We wouldn't want to run time_slice in the base class, I assume adding the underscore prefix in the py filename is to prevent ASV from detecting the file

Related to #19 (comment); more important that it be outside of the nwb_benchmarks/benchmarks testing directory

The prepended underscore is just sklearn import style that effectively says 'the thing I want to publicly expose is the class AcquisitionTimeSeriesSliceBenchmark, not the arbitrarily named submodule it resides in' (which is purely for ease of navigability within the project)

@CodyCBakerPhD CodyCBakerPhD mentioned this pull request Jan 29, 2024
@CodyCBakerPhD CodyCBakerPhD deleted the refactor_functional_some_inheritance 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.

2 participants