Separability of parts of bifs

[RossBoylan]

This is closely related to #15.

Narrow question

@kyoung21b many bump-related items are created only after discovering basis="fourier". Should bumps actually be tied to the basis? The bump info is not saved with the basis object. But it's written as if other bases might use other bump types, or perhaps be unable to use bumps at all.

Broader issue

Also mostlly @kyoung21b. To what extent can we separate concerns into separate spheres? For example, it appears that some of the logic and parameters depend on the likelihood x prior distribution type. There are parameters defined on bifs that seem only to be relevant for some of those, e.g., Rician.

Example 1

The idea is that instead of

        myShape = init_image.shape
        if self.imdim == 1:
            self.kdist = self.bas.kdist1D(*myShape)
            self.k_image = self.bas.tx1(self.init_image) # Get k-space image
        elif self.imdim == 2:
            self.kdist = self.bas.kdist2D(*myShape)
            self.k_image = self.bas.tx2(self.init_image) # Get k-space image
        elif self.imdim == 3:
            self.kdist = self.bas.kdist3D(*myShape)
            self.k_image = self.bas.txn(self.init_image) # Get k-space image

(I've used Python's '*' operator to simplify the existing code) you'd have something like

    self.k_image = self.basis.k_image(self)

where self.basis is some kind of basis object, self is an argument so basis.k_image() can get needed info from the bifs object, and k_image() returns an appropriate k-space image while setting kdist in self.basis.

There is an argument in favor of passing the needed components explicitly to basis.k_image() rather than having it extract them from the bifs object. The advantage of the formulation I gave is that it works better if different classes of basis objects need different info. That's all somewhat theoretical since our mainline only has one possible basis.

Example 2

            if self.prior == "Gaussian" and self.likelihood == "Gaussian":
                self.bifsk_image = self.bifs_map_gauss_gauss(self.prior_mean,self.prior_std2,self.mod_image,self.ksd2)
            elif self.prior == "Gaussian" and self.likelihood == "Rician":
                conj = self.bifs_map_gauss_gauss(self.prior_mean,self.prior_std2,self.mod_image,self.ksd2)
                besind = np.zeros(self.init_image.shape,dtype=int)
                besind[np.where(conj > self.bessel_approx_lims[1])] = 1
                besind[np.where(conj > self.bessel_approx_lims[2])] = 2
                besind[np.where(conj > self.bessel_approx_lims[3])] = 3
                self.bsa = self.bessel_approx_array[besind,:]
                self.bifsk_image = self.bifs_map_gauss_rice(self.prior_mean,self.prior_std,self.mod_image,self.likelihood_scale)
            else:

becomes

   self.bifsk_image = self.likelihood.bifs_map(self)

where self.likelihood is an object, not a string, with a class that depended on the type of prior and likelihood. The rician version would have all the bessel function machinery and Rician-specific parameters tucked away inside of it.

[kyoung21b]

We should discuss those issues. It seemed reasonable to assume that the parameter functions are tied to the basis. I.e. the parameter function is defined over the index space, e.g. k-space for the Fourier basis. Then the bump functions are defined and implemented (added) to the parameter function. While it might be possible (reasonable ?) to abstract the bump functions from the space they’re defined over (index space) that may be a little beyond my pay grade at the moment. The whole idea of a “bump” in wavelet space is pretty complicated (i.e. one can’t straightforwardly define it over scales as in Fourier space because the indices contain spatial information as well - not that figuring that out wouldn’t be a cool research project). Re. splitting up the parameters I’m sure that could use a review. I added things as I developed the code for e.g. new distribution functions (well really just the Rician). The bifs class is a rather large omnibus type class that could perhaps be profitably divided into separate classes, though I’m not sure how straightforward that would be as there’s quit a bit of overlap. Separating out the basis stuff seemed about as far as I could get in terms of obvious things to split out. Sent from this stupid iPhone

…

On Dec 4, 2019, at 5:51 PM, RossBoylan ***@***.***> wrote: This is closely related to #15. Narrow question @kyoung21b many bump-related items are created only after discovering basis="fourier". Should bumps actually be tied to the basis? The bump info is not saved with the basis object. But it's written as if other bases might use other bump types, or perhaps be unable to use bumps at all. Broader issue Also mostlly @kyoung21b. To what extent can we separate concerns into separate spheres? For example, it appears that some of the logic and parameters depend on the likelihood x prior distribution type. There are parameters defined on bifs that seem only to be relevant for some of those, e.g., Rician. Example 1 The idea is that instead of myShape = init_image.shape if self.imdim == 1: self.kdist = self.bas.kdist1D(*myShape) self.k_image = self.bas.tx1(self.init_image) # Get k-space image elif self.imdim == 2: self.kdist = self.bas.kdist2D(*myShape) self.k_image = self.bas.tx2(self.init_image) # Get k-space image elif self.imdim == 3: self.kdist = self.bas.kdist3D(*myShape) self.k_image = self.bas.txn(self.init_image) # Get k-space image (I've used Python's '*' operator to simplify the existing code) you'd have something like self.k_image = self.basis.k_image(self) where self.basis is some kind of basis object, self is an argument so basis.k_image() can get needed info from the bifs object, and k_image() returns an appropriate k-space image while setting kdist in self.basis. There is an argument in favor of passing the needed components explicitly to basis.k_image() rather than having it extract them from the bifs object. The advantage of the formulation I gave is that it works better if different classes of basis objects need different info. That's all somewhat theoretical since our mainline only has one possible basis. Example 2 if self.prior == "Gaussian" and self.likelihood == "Gaussian": self.bifsk_image = self.bifs_map_gauss_gauss(self.prior_mean,self.prior_std2,self.mod_image,self.ksd2) elif self.prior == "Gaussian" and self.likelihood == "Rician": conj = self.bifs_map_gauss_gauss(self.prior_mean,self.prior_std2,self.mod_image,self.ksd2) besind = np.zeros(self.init_image.shape,dtype=int) besind[np.where(conj > self.bessel_approx_lims[1])] = 1 besind[np.where(conj > self.bessel_approx_lims[2])] = 2 besind[np.where(conj > self.bessel_approx_lims[3])] = 3 self.bsa = self.bessel_approx_array[besind,:] self.bifsk_image = self.bifs_map_gauss_rice(self.prior_mean,self.prior_std,self.mod_image,self.likelihood_scale) else: becomes self.bifsk_image = self.likelihood.bifs_map(self) where self.likelihood is an object, not a string, with a class that depended on the type of prior and likelihood. The rician version would have all the bessel function machinery and Rician-specific parameters tucked away inside of it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[RossBoylan]

Thanks for the feedback. So the answer to my narrow question is "Yes, bumps are tied to the basis."

I got into this mostly because of trying to reason about what things need to be updated if various parameters change for #15. If certain parameters only go with certain other choices, e.g., if basis is fourier then there are bumps, if distribution is Rician there are other auxiliary parameters, my inclination would be to keep them out of the main bifs class.

This also affects the documentation, as there are many variables of bifs (the class) that are discussed as if they apply generally, when they don't.

The main complication I can foresee, which is probably present regardless of bifs internals, is that if some parameters are only present some of the time, how does the GUI handle that? I think at the moment it just always allows you to set any parameter, even if the parameter being set will have no effect, like a Rician parameter when you have a Gaussian likelihood.

@kyoung21b it might be good to discuss face to face or eface to eface (like a phone call 😃 ), or maybe in this thread, why you found the wavelet basis didn't fit into the current master branch.

Slightly tangential background: lazy evaluation

In general, the strategy I'm hoping to pursue is lazy evaluation: nothing gets computed until BIFS_MAP. Then if parameters are changed--which would only be via a function call--everything that depends on it is wiped out, only to be computed at the next BIFS_MAP. @kyoung21b Are any of the preliminary calculations expensive?

Simple example of lazy strategy

class X has 3 main variables, a, b, c (like fourier transform of original image, prior distribution, and final image in original space). b depends on a and auxiliary parameters; c depends on a and b and some auxiliary parameters. Then, for example, if an auxiliary parameter for b changes then b and c are set to None. When do_stuff() is called it requests value c via getc(), which requests b via getb() and so on. getb discovers that b is None and recomputes it (otherwise, it just returns the already computed value).

[kyoung21b]

Yes, I think a face to face (or eface to eface) would be good, re. quicker turnaround re. resolving some of these issues. I’m going to be out of town for about a week and a half, so if we want to get in, in that time frame I guess it would have to be eface to eface. After that I’m happy to show up at whichever UCSF spot is best for you. Re. the structural stuff (e.g. whether/when to blast the src directory), for the moment I’m planning on leaving that in your and David’s more capable hands re. decisions on the “final” package structure. Re. parameter setting, the philosophy was (though perhaps poorly implemented) that most parameters are set by default. The GUI has access to some subset of the parameters, i.e. the “higher level” type parameters (e.g. what distributions to use for likelihood and prior, the variances for those, what parameter function to use, and whether to add filters (“bumps”)). To gain “lower level” access one would have to use the bifs class from the command line; then all parameters are available. The easiest way to implement that (and, e.g. delay decisions about what to allow access to and what not to in the GUI) seemed to be to just throw everything into the top (bifs) class. I tried to put some checks in re. parameter dependencies, i.e. if some parameter changed, then parameters that depended on it had to be updated but I no doubt missed some. If I understand your suggestion, that might be better implemented by defining something like subclasses. If that is indeed your suggestion it’s probably a good one but there are are a lot of overlapping dependencies and it might be hard to implement that. — K Karl Young

…

On Dec 4, 2019, at 9:30 PM, RossBoylan ***@***.***> wrote: Thanks for the feedback. So the answer to my narrow question is "Yes, bumps are tied to the basis." I got into this mostly because of trying to reason about what things need to be updated if various parameters change for #15 <#15>. If certain parameters only go with certain other choices, e.g., if basis is fourier then there are bumps, if distribution is Rician there are other auxiliary parameters, my inclination would be to keep them out of the main bifs class. This also affects the documentation, as there are many variables of bifs (the class) that are discussed as if they apply generally, when they don't. The main complication I can foresee, which is probably present regardless of bifs internals, is that if some parameters are only present some of the time, how does the GUI handle that? I think at the moment it just always allows you to set any parameter, even if the parameter being set will have no effect, like a Rician parameter when you have a Gaussian likelihood. @kyoung21b <https://github.com/kyoung21b> it might be good to discuss face to face or eface to eface (like a phone call 😃 ), or maybe in this thread, why you found the wavelet basis didn't fit into the current master branch. Slightly tangential background: lazy evaluation In general, the strategy I'm hoping to pursue is lazy evaluation: nothing gets computed until BIFS_MAP. Then if parameters are changed--which would only be via a function call--everything that depends on it is wiped out, only to be computed at the next BIFS_MAP. @kyoung21b <https://github.com/kyoung21b> Are any of the preliminary calculations expensive? Simple example of lazy strategy class X has 3 main variables, a, b, c (like fourier transform of original image, prior distribution, and final image in original space). b depends on a and auxiliary parameters; c depends on a and b and some auxiliary parameters. Then, for example, if an auxiliary parameter for b changes then b and c are set to None. When do_stuff() is called it requests value c via getc(), which requests b via getb() and so on. getb discovers that b is None and recomputes it (otherwise, it just returns the already computed value). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTQY7RCCREXCZC2W5VBBWLQXCGYHANCNFSM4JVSW4IA>.

[RossBoylan]

Summarizing some other relevant comments from others, possibly inaccurately 😃

Karl: wavelets proved different enough from a fourier basis that they have moved into a separate product.

John K: Many choices are a function of the combination of the prior and the likelihood; some combinations may not be present, either because they are nonsense or because we can't handle them.