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

Simplifying datastore.py #158

Open
BSchilperoort opened this issue Feb 20, 2023 · 2 comments
Open

Simplifying datastore.py #158

BSchilperoort opened this issue Feb 20, 2023 · 2 comments
Labels
enhancement New feature or request
Milestone

Comments

@BSchilperoort
Copy link
Collaborator

The datastore.py module has become extremely large (6000 lines 😵‍), and has all the DataStore class methods explicitly implemented in the file. This makes development more difficult.

One relatively easy way to improve this, is to split of the method implementations into different files. For example, make a new file calibration_routines/double_ended.py, and refer to this in the DataStore implementation:

from dtscalibration.calibration_routines import double_ended, single_ended

class DataStore:
    ...
    def calibration_single_ended(self, **kwargs):
        single_ended.calibrate(self, **kwargs)

    def calibration_double_ended(self, **kwargs):
        double_ended.calibrate(self, **kwargs)
@BSchilperoort BSchilperoort added the enhancement New feature or request label Feb 20, 2023
@bdestombe
Copy link
Collaborator

Sounds very reasonable! All for keeping the codebase manageable.

@bdestombe
Copy link
Collaborator

To what extent do we actually need the DataStore class as subclass of Xarray's Dataset?
What about instead of subclassing to add a dts accessor that holds the calibration functions? https://docs.xarray.dev/en/stable/internals/extending-xarray.html

  • ds.dts.single_ended_calibration()
  • ds.dts.double_ended_calibration()
  • ds.dts.sections and its values are still stored in ds.attrs["_sections"]
  • ds.dts.variance_stokes()
  • ds.dts.__repr__()

Taking your code it would look as follows:

from dtscalibration.calibration_routines import double_ended, single_ended

@xr.register_dataset_accessor("dts")
class DataStore:
    ...
    def calibration_single_ended(self, **kwargs):
        single_ended.calibrate(self, **kwargs)

    def calibration_double_ended(self, **kwargs):
        double_ended.calibrate(self, **kwargs)

This entire overhaul is much bigger than what you propose.. Maybe leave it at your proposal for now. I am doing this thing with the accessor with pandas object and it works really neat! Keeps everything tidy and allows for combining different accessors simultaneously.

@bdestombe bdestombe added this to the v3.0.0 milestone Jul 30, 2023
@bdestombe bdestombe mentioned this issue Aug 4, 2023
5 tasks
@bdestombe bdestombe modified the milestones: v3.0.0, Accessor Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants