search

esi-neuroscience / syncopy

Systems Neuroscience Computing in Python: user-friendly analysis of large-scale electrophysiology data
BSD 3-Clause "New" or "Revised" License
48 stars 14 forks source link

Rename `ComputationalRoutine` and `computeFunctions`? #236

Closed joschaschmiedt closed 1 year ago

joschaschmiedt commented 2 years ago

Problem In the original Syncopy concept, one of the design ideas was to separate the "algorithmic" level of a computational method from the management part that deals with I/O and parallelization. This separation should make code development easier, in particular for scientists. In order to achieve this, a computational method operating on data was supposed to consist of

  1. the computeFunction(), which should operate on few numpy arrays and return (a) numpy array(s)
  2. the wrapping ComputationalRoutine class that deals with "by executing all necessary auxiliary routines leading up to and post termination of the actual computation (memory pre-allocation, generation of parallel/sequential instruction trees, processing and storage of results, etc."

After more and more implementations of computational methods this separation seems to simplistic to actually work well. Having only one computeFunction per method encourages writing long functions dealing with many things, making testing very difficult. This was already alleviated by @tensionhead's great work splitting backend and frontend.

Looking at it now, to me, the names ComputationalRoutine and computeFunctions don't seem to catch what these are actually doing.

Suggestion I'd suggest do change the names to match their functionality better.

As the ComputationalRoutine deals with I/O and parallelization I'd suggest

For the computeFunction I'd suggest

@tensionhead and @pantaray , what do you think? Any other suggestions?

tensionhead commented 2 years ago

Thanks for your detailed thoughts and analysis about this topic @joschaschmiedt ! I was also wondering what is the best way.. if our frontends would be super nicely designed, I could imagine to get rid of hte computeFunction entirely and just use another decorator to get things like noCompute done and leave everything else to the ComputationalRoutine (here I already really like the renaming to MethodManager :+1:). If we see that this is not possible, than also renaming the computeFunction seems to be a good idea indeed!

However the biggest issue for me at the moment is #140, this should (and will get) addressed before we do anything else imho.

pantaray commented 2 years ago

I agree, renaming ComputationalRoutine and computeFunction makes a lot of sense at this point. I also like MethodManager (since we use it for sequential computations as well), for the computeFunction the methodFrontend sounds quite good in my opinion. I think renaming these components is independent from the multiple output issue @tensionhead ?

tensionhead commented 2 years ago

Welll, it might be that soonish we have not only one ComputationalRoutine or MethodManager.. and also as I said, maybe the computeFunctions can go alltogether then. So I still think we should re-brand only once we now what we want to do. It's also not only the multiple output issue, but for example a general recipe to tackle things like the PPC. At the moment that would only work by writing computeFunctions which all of a sudden don't operate on a simple array input anymore..

tensionhead commented 1 year ago

So far this naming convention served us well, so there is no apparent need for change.