EconForge / interpolation.py

BSD 2-Clause "Simplified" License
124 stars 35 forks source link

move complete_polynomial stuff from dolo: #13

Closed sglyon closed 8 years ago

albop commented 8 years ago

@spencerlyon2 : this is very welcome. I'll have a look at it tomorrow.

sglyon commented 8 years ago

Great, thank you!

albop commented 8 years ago

I looked at this code again and have a few comments (we should wait for 1/ to merge the code and track the other points in separate issues) 1/ the test does nothing 2/ this is specific to dimensions <= 5. It would be nice to generate code for higher order dimensions automatically with a call to @generate 3/ even so, it's not clear the algo would scale up very far (basis matrix is dense for instance) 4/ The more I think about it, the more it looks like we should have some kind of unified way of dealing with various types of base products (cartesisan, complete, smolyak). I'll try to make some plans.

sglyon commented 8 years ago

The code should almost certainly be cleaned up.

We definitely need tests, good catch.

Also, the algorithm itself isn't very efficient. Higher order terms repeat lower order products, when they could be reused. (e.g. the 5th order term x^5 is computed as x*x*x*x*x instead of x^4*x when we already have x^4 computed).

Any plans on how to generalize/unify these products is very welcome.

albop commented 8 years ago

@spencerlyon2 : let's rediscuss the implementation when we have a clear plan the various products of linear bases

albop commented 8 years ago

@spencerlyon2 , I did two modifications to your PR:

I also played a bit with code generation to improve the current proposal but it's not finished yet. I suggest we merge as it is now, and when a better solution emerges, move complete_poly.py to a more permanent location.

albop commented 8 years ago

Just to be clear, I regard this code as half experimental but useful. In particular, in the object wrapper, I did not try to manage the memory. If it happens to be a bottleneck, it will be easy to fix.

sglyon commented 8 years ago

Thanks for updating this. I think the changes you added are great.

I would also like to polish this up a bit as I believe we can come up with a better algorithm.

I'm in favor of merging now.