alexjbest / cluster-pictures

This package implements the machinery of cluster pictures of Maistret, Morgan, Dokchitser and Dokchitser in Sage.
https://alexjbest.github.io/cluster-pictures/
GNU General Public License v2.0
3 stars 4 forks source link

avoid use of symbolic functions and simplify #47

Closed fchapoton closed 2 years ago

fchapoton commented 2 years ago

For efficiency, it is better to avoid use of symbolic functions and symbolic ring whenever possible.

I have therefore remove the use of min_symbolic and simplify.

fchapoton commented 2 years ago

also, it's recommended to avoid mutable default values.

fchapoton commented 2 years ago

ah, I see now that this may be required when using symbolic variables in the matrix input..

fchapoton commented 2 years ago

maybe it would still be possible to avoid simplify by forming the set of values before the min_symbolic ?

alexjbest commented 2 years ago

Yes the symbolic variables are not ideal, but currently we do not have a better formalism for examples like https://alexjbest.github.io/cluster-pictures/cluster_pictures.html#sage_cluster_pictures.cluster_pictures.BYTree.tamagawa_number to work. These examples are a really important / useful feature of our implementation. The fact those tests still pass is encouraging but I worry that with this change some less trivial examples will print worse if the input depths aren't simplified. I'd like to try and reconstruct my our original reason for using simplify_full before merging this, I remember we tried a few different things before hitting upon the thing that works, so I'm a little worried our test cases might just be missing some examples.

In the long term what we really need is a custom class for the exponents, which behaves like some sort of symbolic variables, but where we can also add constraints like being even or odd, I don't know of any way out of the box to do this in Sage. The present approach is sub-optimal but was the easiest way to proceed at the time. I definitely

fchapoton commented 2 years ago

ok, then let's just close this pull request without merging. I did try a few other changes, with no satisfaction