Abstract base class for analysis functions

[hombit]

Here I introduce an abstract base class AnalysisFunction for built-in analysis functions, which could also be used to create user's analysis functions. The instances of its sub-classes can be evaluated via calculate() method.

It also renames currently available analysis function removing calc_ prefix from them, calc_stetson_J -> stetson_J, calc_sf2 -> sf2.

Closes #200

[dougbrn]

This look great @hombit. My main request is to update the docs where a few lingering issues remain, particularly there's a few instances where it looks like calc_sf2 is called on array data, and those should be changed to calc_sf2.calculate calls.

[hombit]

@dougbrn thank you! BTW, I'm not really happy about this .calculate() method: calc_sf2.calculate sounds weird. How do you think, could we replace it with __call__() or rename calc_sf2 -> sf2 and calc_stetson_J -> stetson_J?

[dougbrn]

@dougbrn thank you! BTW, I'm not really happy about this .calculate() method: calc_sf2.calculate sounds weird. How do you think, could we replace it with __call__() or rename calc_sf2 -> sf2 and calc_stetson_J -> stetson_J?

Hmm, I'm not sure that __call__() is ideal, as users may be annoyed/confused by the instantiation needed:

class AnalysisFunction:
    def __init__(self):
        print("Instance Created")

   def columns(self):
       return("flux")
  
    # Defining __call__ method
    def __call__(self, a, b):
        print(a * b)
  
# Instance created
ans = AnalysisFunction()
  
# __call__ method will be called
ans(10, 20)

# but we still can't just directly use this as a function...
AnalysisFunction(10,20)

Renaming the calc functions seems more reasonable, it looks like you currently have calc_stetson_J being imported as it's assigned as calc_stetson_J = StetsonJ, can we just let users directly import the StetsonJ class?

[hombit]

I agree, with the only correction: currently calc_stetson_J = StetsonJ(), so calc_stetson_J is a callable object, an instance of StetsonJ class. Both things are importable, but I don't see how the class could be useful, because it has a trivial constructor.

[dougbrn]

Alternatively, we could define the "calculate" function as a staticmethod of the class, like this:

class AnalysisFunction:
    def __init__(self):
        print("Instance Created")

    def columns(self):
       return("flux")
  
    # Defining __call__ method
    @staticmethod
    def analysisfunction(a, b):
        print(a * b)
  
# Call via Ensemble -- uses the class
ens.batch(AnalysisFunction)

# Call as function independently -- uses the staticmethod
AnalysisFunction.analysisfunction(10,20)

But this may be annoying to users as well that attempt to pass the static method to the ensemble, or try to act on the class like a function.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[dougbrn]

Ah, this is tricky. Calculate should not be added here, as sf2 is a function of the timeseries class as invoked here. There's a few instances of that throughout this doc. It looks like timeseries.sf2 and ensemble.sf2 have also not been updated with the rename. As a future note, we may want to deprecate the unique sf2 functions in ensemble and timeseries in favor of a different batch function that serves a "glob" of lightcurves to a function.

Reply via ReviewNB

[hombit]

Thank you, I did too mechanically! I'll fix it

[codecov]

Codecov Report

Merging #206 (9969b18) into main (0bf5314) will decrease coverage by 0.76%.
The diff coverage is 90.74%.

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
- Coverage   92.95%   92.20%   -0.76%     
==========================================
  Files          20       21       +1     
  Lines        1051     1091      +40     
==========================================
+ Hits          977     1006      +29     
- Misses         74       85      +11     

Files Changed	Coverage Δ
src/tape/analysis/base.py	75.00% <75.00%> (ø)
src/tape/analysis/stetsonj.py	85.24% <78.37%> (-6.43%)	⬇️
src/tape/analysis/structurefunction2.py	96.85% <97.08%> (+0.23%)	⬆️
src/tape/analysis/__init__.py	100.00% <100.00%> (ø)
src/tape/ensemble.py	88.96% <100.00%> (-0.47%)	⬇️

[dougbrn]

Nice, this looks good now with all the CI squared away!

[hombit]

@dougbrn Should I improve test coverage?

[dougbrn]

If you have ideas for further tests, feel free to add them. But I don't think that any of the loss in coverage here is especially impactful or merge-preventing

[hombit]

I added a couple of new tests, but not going to add anything else

[nevencaplar]

I talked with @hombit today; and I just reiterate that the main functionality that I want to have preserved is that an user can invoke our ``special functions'' just as StetsonJ or `sf2` without invoking the whole TAPE ensemble. I think we all are behind that and understand that. i know that is not the main point of this pull, but I find it so important that I am willing to repeat myself.

In best case scenario the flow is

t = [1, 2, 3]
y = [2, 3, 4]
yerr = [3, 4, 5]
result = tape.sf2(t,y,yerr)

I understand that this pull changes this. I think that I am fine with both options of result=tape.analysis.sf2.calculate(t, y, yerr) and result=tape.analysis.sf2.sf2(t,y,yerr) and result=tape.analysis.sf2(t,y,yerr) (I believe that is the point of the last discussion). I think in tutorial we want to probably simplify that with invocation of import tape.analysis.sf2 as sf2.

[hombit]

@dougbrn and @nevencaplar. I've just pushed the code to 1) rename functions back to calc_*, and 2) to rename calculate() to __call__, so finally this PR provides no interface change for analysis functions. If you are ok with this, I rebase and merge it

[dougbrn]

I'm in favor of the change to preserve the current interface for analysis functions. I just have one last question for the ensemble.batch side, doesn't this implementation mean that a user should supply the StetsonJ class to batch, as supplying the calc_stetson_J instance will not be picked up by ensemble.batch's check to see if it's a subclass of AnalysisFunction? If this is the case, we should point this out in the documentation as much as possible, as I can see users easily just using the calc_stetson_J instance with the ensemble.

[hombit]

Actually this PR changes nothing in how we use analysis functions with batch. New batch implementation checks if the input function is an instance of AnalysisFunction, and both calc_sf2 and calc_stetson_J are instances of AnalysisFunction's child classes:

In [2]: from tape.analysis import calc_stetson_J, AnalysisFunction

In [3]: isinstance(calc_stetson_J, AnalysisFunction)
Out[3]: True

THat means that there is no need to use these classes directly, and the only reason I keep them as a part of the "public" interface is too allow subsequent inheritance.

[dougbrn]

Oh! I see, that's really cool.