artofscience / SAOR

Sequential Approximate Optimization Repository
GNU General Public License v3.0
5 stars 1 forks source link

Restructure intervening variables #43

Closed MaxvdKolk closed 3 years ago

MaxvdKolk commented 3 years ago

From the commit:

The different intervening variables are slightly reorganised and split
into different files. The linear and reciprocal variants are hereby
expressed using their more general exponential variant to minimise
repeated definitions of all functions, making the classes such as
`Linear`, `Reciprocal`, etc., simply a convenient wrapper for the user.

The more extensive intervening variables are moved into their own files,
such as ConLin and the MMA variants. Additionally, the ones using fits
are placed in a single file `fitted.py` and might be moved to separate
files later if needed.

Additionally, the `__init__.py` is populated with some imports, to
simply the importing of the classes from `from
sao.intervening_variables.conlin import ConLin` to `from
sao.intervening_variables import ConLin`. This should also make it
somewhat easier to changes the underlying files without the need to
change the import paths.

Finally, some basic documentation is added.

I will see if I can work a bit more on these (and related) files, but just let me know what you think of the current changes @artofscience @Giannis1993.

MaxvdKolk commented 3 years ago

What do you think about changing the broadcasts to short loops over the rows of the variables? Would need to check if this has a performance hit though, but it might improve readability? I find the broadcasts can be come quite tricky to read 😅 . Can be extended to all the other intervening variables of course.

I was wondering if there was a way to simplify the functions more, as the y, dydx, ddyddyx are all basically the same except for the function they map across the rows. But it feels it would be more complex doing something like the following

def map(self, x, out, f, g):
    for (row, positive) in self.positive:
        out[row, positive] = f(x[positive])
        out[row, ~positive] = g(x[~positive])
    return out 

def y(self, x): 
    y = np.zeros_like(x)
    return self.map(x, y, self.lin.y, self.rec.y)

def dydx(self, x)
    dydx = np.zeros(x)
    return self.map(x, y, self.lin.dydx, self.rec.dydx)
Giannis1993 commented 3 years ago

What do you think about changing the broadcasts to short loops over the rows of the variables? Would need to check if this has a performance hit though, but it might improve readability? I find the broadcasts can be come quite tricky to read 😅 . Can be extended to all the other intervening variables of course.

I was wondering if there was a way to simplify the functions more, as the y, dydx, ddyddyx are all basically the same except for the function they map across the rows. But it feels it would be more complex doing something like the following

def map(self, x, out, f, g):
    for (row, positive) in self.positive:
        out[row, positive] = f(x[positive])
        out[row, ~positive] = g(x[~positive])
    return out 

def y(self, x): 
    y = np.zeros_like(x)
    return self.map(x, y, self.lin.y, self.rec.y)

def dydx(self, x)
    dydx = np.zeros(x)
    return self.map(x, y, self.lin.dydx, self.rec.dydx)

I remember discussing with @aatmdelissen and he told me that the for loop in Python is pretty slow. Since computing y(x), dydx(x), ddyddx(x) is performed every time g_approx is calculated (and that is several times per design iteration) I thought that the fastest way would be to use some numpy function. I agree though that, if it doesn't mess up the performance, it would be much easier to read.

MaxvdKolk commented 3 years ago

Ah yes, that's true indeed. In that case I we could arrange them as follows:

out = np.zeros_like(x) 
lin = self.lin.y(x)
rec = self.rec.y(x)
for (row, positive) in self.positive: 
    out[row, positive] = lin
    out[row, ~positive] = rec 
return out 

Not sure if it makes much difference, as I suppose that broadcast is doing this in a similar way (as it maps the computed result to all rows of the output) but it could be more readable?

I will play around a bit more with this to see how we could arrange it :-)