CachingCalculator class

[lukfugl]

This is a crazy bit of metaprogramming that I dreamt up while in class today. I understand if it's too scary for actual use. But I wanted to get some other eyeballs on it, and I really do think it could be useful if polished.

So tear it apart, criticize it, tell me why it won't work. You won't hurt my feelings. I've already identified one major shortcoming myself in the comments and commit message.

But if it will work, hopefully my evening was more than just an entertaining exercise. :)

[Aldriana]

This does seem like sort of a cool idea in theory, though I admit that I'm not sure how useful its going to be in practice - the applications aren't entirely obvious to me, though that might be partly because I've been making an active effort to avoid recalculating stuff when I don't need to. I'd also feel a little more comfortable if we had a sample implementation in the existing code paths to verify that it responds correctly for things like EP calculations.

Regardless, I'll look over it when I have time, but as I don't have the faintest idea what this is doing it'll take me some time to work through this, and frankly sorting through elaborate optimizations is a lower priority for me right now than getting more-or-less functional models for all 3 classes going, so it might be a couple of days (or even a week or two) before I get around to it. If anyone else can review and comment in the meantime, that would be helpful - not that I don't trust you, but for any complicated piece of code like this I figure its always better to get as many eyeballs on it as possible.

[lukfugl]

I completely understand. Take your time. The ExampleCachingCalculator in the test class shows a little bit how it would look, but it's obviously contrived. I'm looking for a prime example of some of the existing code that could be simplified by it, but the inter-object failings and the lack of parameters is making it a little less applicable in its current form.

The basic idea is that you can write your calculations as if everything the calculation depended on were an attribute and as if you didn't care about recalculating (the code looks like it would recalculate on every call; the caching is transparent).

I'll try and push and example in the near future.

[dazer]

I'm still trying to break through it as I told you in my pm. The most interesting application I can think of would be in the EP compute: it passes over the same formulae 1 time per stat (multiplied by an average of 3 over get_dps due to the 10E-7 convergence).

[julienp]

Reading through the code, I don't see any bugs or mistakes and it's certainly a nice example of python meta-programming, but does it actually solve a problem we have? And is it actually faster?

In the normal variant of Buffs with explicit calculations, each time we check for a buff we have one function call and one actual calculation.

For the cached variant, ignoring the first calculation of each buff, each time we check for a buff the descriptor __get__ gets called, we have a lookup and a function call for self.cache.caller(), a lookup and a call for self.cache.has_valid(cache_key) and finally another lookup and call for self.cache.get(cache_key).

IMO the caching calculator only makes sense where the actual calculations are somewhat expensive and I don't think we have any of those in ShadowCraft. Most are just a couple of floating point operations, which I doubt take more time than several attribute lookups and function calls.

If we later see that certain functions like possibly get_cp_distribution_for_cycle end up taking the biggest chunk of CPU time, we can look at optimizing them individually. I'd personally prefer explicit caching on a per function basis where it actually matters instead of having magic caching that might or might not speed things up and possibly makes things harder to follow and/or debug.

This is of course just my personal opinion based on a pretty simplistic view and might be flawed.

[Aldriana]

So, I think consensus is that this is not something we really need; its certainly cool, but as julienp says, I don't think it really solves any issues we're having. Hence closing.