porespy icon indicating copy to clipboard operation
porespy copied to clipboard

Added logger decorator to all functions

Open Daniel-olaO opened this issue 1 year ago • 6 comments

close #838

Daniel-olaO avatar May 24 '23 15:05 Daniel-olaO

@Daniel-olaO I prefer the attribute approach. Adding decorators to every function, although is more explicit, but is less maintainable.

ma-sadeghi avatar May 24 '23 16:05 ma-sadeghi

(especially since it's just for logging, if it were for something more substantial like just-in-time compilation, adding the decorator to function header would make sense since developers should really know what's going on, whereas for logging, the passive approach seems more sensible to me), but let's wait for @jgostick to weigh in before you make the changes to not waste your time. Also, I really appreciate your efforts :)

ma-sadeghi avatar May 24 '23 16:05 ma-sadeghi

okay thanks @ma-sadeghi

Daniel-olaO avatar May 24 '23 16:05 Daniel-olaO

There are pros/cons for both approaches. The decorator is nice because it let's us explicitly pick and choose which functions to emit the logger message from. I think @Daniel-olaO has gone a bit overboard by putting them on ALL the functions. If we just do it on the really heavy functions, then I think the maintenence would not be too bad, AND the approach would be explicit. On the otherhand, @ma-sadeghi's module attribute approach only requires code in ONE place, which is really tight. The downside of this approach to me is that it also represents a maintenence issue in the future when we have to ask ourselves WTF these logger messages are coming from!

jgostick avatar Jun 02 '23 10:06 jgostick

hI @jgostick, why not write a list of functions you want me to add the logger decorator

Daniel-olaO avatar Jun 06 '23 12:06 Daniel-olaO

Another perspective: do we even need this? Profiling should be done by the user, and there are many great tools that exactly does this for us: scalene, memprofiler, etc. They provide per function runtime, memory usage, and even nice flame graphs.

ma-sadeghi avatar Jun 06 '23 13:06 ma-sadeghi