♻️ Refactoring Sample container

[bstaber]

Here's a proposal regarding the refactorization of Sample. The idea is to end up with a Sample such as:

from plaid.containers.sample import ScalarManager, FieldManager

class Sample:
     
   def __init__(self):
         self.scalars = ScalarManager()
         self.fields = FieldManager()
         self.time_series = TimeSeriesManager()
        ...

And to have a layout such as:

└── src/plaid/containers/sample
    ├── scalar_manager.py             # Implements ScalarManager
    ├── field_manager.py               # Implements FieldManager
    ├── time_series_manager.py    # Implements TimeSeriesManager
    └── sample.py                          # Implements Sample

I pushed (empty) examples for ScalarManager and TimeSeriesManager just for opening the discussion. I haven't done TimeSeriesManager but it would be similar. What do you think about the naming (manager) and layout ? Once we agree on something, I can start implementing (mostly copy pasting I think).

[gitnotebooks]

Review these changes at https://app.gitnotebooks.com/PLAID-lib/plaid/pull/125

[codecov]

Codecov Report

❌ Patch coverage is 0% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...plaid/containers/sample_container/field_manager.py	0.00%	12 Missing ⚠️
...laid/containers/sample_container/scalar_manager.py	0.00%	12 Missing ⚠️

📢 Thoughts on this report? Let us know!

[xroynard]

Nice idea !

We could even have:

from plaid.containers.sample import FeatureManager

class Sample:
     
   def __init__(self):
         self.features: list[FeatureManager] = []
        ...

But how would requests to a Sample instance be given to the underlying *Managers ?
Every method of Sample would be trivial such as ?

class Sample:
     
   def get_scalar(self, name):
      return self.scalars.get_feature(name)

[bstaber]

Yes I guess that you can choose which methods you want to expose in Sample and which methods you keep "hidden" in the *Manager

[casenave]

Nice ! Having sample's only parameters self.features would require many changes, but it would be easier to maintain et make evolve, as well as having more standard handling of features as proposed in #101

[xroynard]

About *Managers, the idea would be to have a BaseManager with virtual methods, to at least define the minimal API available for a feature ?

for example:

• get
• add
• get_names
• remove
• rename

---

About the Sample class, do you think it is possible build a method at runtime ?

let me explain, for example the method get_scalar does not exist, but when a user uses my_sample.get_scalar(name), some code somewhere sees the name of the method get_scalar and calls self.scalars.get(name).
If it sees get_field it calls self.fields.get(name) ...

[xroynard]

It seams doable with __getattr__:

class A(object):
      def __init__(self):
          self.val = {'a':1, 'b':2}
      def __getattr__(self, attr):
          if attr=='get_scalars':
             def fun(name):
                 return self.val[name]
             return fun
          else:
             raise AttributeError(f"'A' object has no attribute '{attr}'")

a = A()
a.get_scalars(name)

[bstaber]

About *Managers, the idea would be to have a BaseManager with virtual methods, to at least define the minimal API available for a feature ?

for example:

• get
• add
• get_names
• remove
• rename

Yes we could do that, the only issue I see is that the methods don't have the same signatures accross feature types. What would be the signature of the get method in the base class ?

def get(self) -> T:
    raise ...
    
def get(self, name: str) -> T:
    raise ....
    
def get(self, name: str, **kwargs) -> T:
    raise ....

[bstaber]

It seams doable with __getattr__:

class A(object):
      def __init__(self):
          self.val = {'a':1, 'b':2}
      def __getattr__(self, attr):
          if attr=='get_scalars':
             def fun(name):
                 return self.val[name]
             return fun
          else:
             raise AttributeError(f"'A' object has no attribute '{attr}'")

a = A()
a.get_scalars(name)

That's cool ! I think we could look at that after everything has been simply reorganized