Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speeding up composition of statically defined functions #397

Open
ott2 opened this issue Aug 3, 2018 · 2 comments
Open

speeding up composition of statically defined functions #397

ott2 opened this issue Aug 3, 2018 · 2 comments

Comments

@ott2
Copy link
Collaborator

ott2 commented Aug 3, 2018

It is often convenient to describe transformations in terms of functions, especially when dealing with several transformations that can be written as compositions of more basic ones. Currently, Conjure turns deeply nested compositions into complex expressions (taking a long time to do so), and then Savile Row has to deal with these complex expressions in turn (also taking a long time to do so).

Here is an extreme example: the function g rotates oriented symbols by 90 degrees, so four applications of the function is the identity and should return the original symbol. It is actually possible to simplify the entire spec to true.

Although Conjure is very slow here (6 minutes), note that SR+Minion still takes 19s so we can't just amortise the translation cost.

$ demonstrating really slow function composition
$ 383.4s initially, 19.2s subsequent
$ Conjure leaves the static functions as-is, accumulating increasingly
$ complex expressions, instead of computing composition
letting dir be new type enum {NE,NS,NW,ES,EW,SW}
letting g be function (NE-->ES, ES-->SW, SW-->NW, NW-->NE, EW-->NS, NS-->EW)
letting N be [[ES, EW, EW, SW; int(0..3)], [NE, EW, SW, NS; int(0..3)],
[ES, EW, NW, NS; int(0..3)], [NE, EW, EW, NW; int(0..3)]; int(0..3)]
find C : matrix indexed by [int(0..3),int(0..3)] of dir such that
   forAll i,j : int(0..3) . C[i,j] = g(g(g(g(N[i,j]))))
$ expression should simplify to true, but does not

In contrast, moving the function to a parameter file is reasonably fast (although still much slower than one would hope, given that it should simplify to true):

$ demonstrating fast function composition when function is parameter
$ 2.9s initially, 2.4s subsequent
$ here Conjure computes composition and simplifies it correctly
letting dir be new type enum {NE,NS,NW,ES,EW,SW}
given g : function dir --> dir
letting N be [[ES, EW, EW, SW; int(0..3)], [NE, EW, SW, NS; int(0..3)],
[ES, EW, NW, NS; int(0..3)], [NE, EW, EW, NW; int(0..3)]; int(0..3)]
find C : matrix indexed by [int(0..3),int(0..3)] of dir such that
   forAll i,j : int(0..3) . C[i,j] = g(g(g(g(N[i,j]))))
$ expression simplifies to true

with associated parameter file:

letting g be function
   (NE-->ES, ES-->SW, SW-->NW, NW-->NE, EW-->NS, NS-->EW)

I suggest that the composition of two statically defined functions should be simplified, by creating an auxiliary function for their composition. With this change, the statically defined function could be simplified away, although the second case would probably have to be treated in the same way it is now.

@ozgurakgun
Copy link
Collaborator

Hi Andras,

As discussed before: Conjure's modelling mode isn't really suited for handling large amounts of data. (I realised these functions aren't very large, but alas). However when the data is in the parameter file (and when the specification itself treats them as symbolic functions) it is handled much much better.

Regarding your specific request: I do not consider this as a major limitation and not something we need to improve immediately. To be explicit: the suggested workaround is copying the data out from the model file into the parameter file. That being said, I would happily merge in pull requests that add a special rule to achieve what you suggest above though.

Leaving this issue open, but I might mark it as low priority (if I find out how to do it).

@ott2
Copy link
Collaborator Author

ott2 commented Aug 11, 2018

Hi Oz -- note this is a FR, not a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants