Loading different images in the same session

[RossBoylan]

Scenario

In the GUI load one image, work with it, then load another image, possibly with different dimensions or other attributes.

Or load repeated images without the GUI.

Check

The second image is set the same way it would have been if it were loaded first.

Concern

The second image might carry over some settings from the first, or be missing some settings.

Background

Because of these concerns, Karl initially recreated self.mybifs with each new image load. Ross changed this to rely more on copying. I (Ross) think the motivations were two-fold:

1. Code reuse. bifs_gui.py doMap had code for creating a new bifs object based on an old one. I needed that same logic elsewhere, for scanning images to get an empirical prior. So I moved the logic to a separate function (in bifs.py, since there's nothing GUI specific in it).
2. Performance: many fresh allocations used lots of memory and were slow (not sure about this one).

In particular, commit 47f9171 introduced this change, with the cautionary note

            # Reinitialize bifs object but keep current parameter setttings
            # RB: I refactored the parameter copying to the bifs object itself.
            # However, this means I set the parameters and then load the image file.
            # The original code loaded the image file and then set the parameters.
            # Since I don't understand why the reinitialization was necessary at all,
            # it's possible this change in sequence will break something.
            self.mybifs = self.mybifs.copy_params()

Extra Credit

Create one or more automated tests for these issues.

[RossBoylan]

This note is about what happens when an image is loaded. This is relevant both to the question off what processing is necessary achieve a clean slate, the topic of this issue, and to my current efforts to generate images that have been processed in a variety of ways.

1. Whenever someone requests a run of the bifs processing this triggers bifs_gui.py:doMAP, which calls bifs.py:load_image_file, which calls bifs.py:load_image.
2. load_image_file rereads the file from disk, which seems wasteful. Other than that, it has no direct dependence on any user-settable parameters.
3. But load_image does: it depends on the selected basis (which can only be Fourier for now) and the parameter function. Also ...
4. load_image invokes set_prior_from_param_func which additionally use the scale and standard deviation parameters. It also depends on bumps if present.
5. From the perspective of my current exercise of running many different parameters against the same image it seems unfortunate I must reread the input file each time to be sure the setup logic is right.
6. From the perspective of whether my substitution of a copy for a new bifs object creation is safe, I'm not sure. There is stuff done in bifs.__init__ that depends on the basis and parameter function types. So if a user loads an image and then changes the parameter function type in bifs_gui, copying the bifs object will not properly set up the copy. I don't know that the old code would either, and it is possible that some later processing will fix it up anyway.
7. One approach to handling these dependencies is that values in bifs should only be changed through setter functions(*) and those functions would manage any necessary updates. I think we mostly are already using setter functions. They do not currently manage the dependencies (if parameter x changes than y and z must be updated) systematically, which is why there is all this recreation and re-initialization code.
8. Instantaneous updates may be undesirable because the relevant calculations are slow. Updates may be impossible if, for example, changing one value such as parameter function type (switch to empirical prior) requires another value to be set (file name of the empirical prior--not how it works now, but it could). In either case the solution would be to set an appropriate "dirty" flag and recompute later on, such as when a map is requested. The first cut would be a single dirty indicator that caused a complete setup to happen; more refined approaches are possible.

(*) I'm pretty sure Python allows you to do some tricks so an apparently non-functional setting like
a.b = 33 actually invokes a user-defined setter function behind the scenes. By "setter functions" I mostly meant the more explicit a.setB(33).

[RossBoylan]

Point 6 above says

it is possible that some later processing will fix it up anyway.

bifs.BIFS_MAP includes this code, which does at least some of the fixup:

            # In case parameter function or decay value were reset:
            self.set_prior_from_param_func(self.param_func_type)

That method does do some fixes, though it does not redo the settings of

            self.param_func_type = self.bas.param_func_type
            self.param_func_choices = self.bas.param_func_choices
            if self.param_func_type == "Inverse Power Decay":
                self.bvec = self.bas.bvec_ixsc
            elif self.param_func_type == "Banded Inverse Power Decay":
                self.bvec = self.bas.bvec_ixscbanded
            elif self.param_func_type == "Linear Decay":
                self.bvec = self.bas.bvec_linsc

in the __init__ method. However, set_prior_from_param_func_type has code like this:

                if pft == "Inverse Power Decay":
                    self.prior_mean = self.bas.ixsc(self.bvec,self.kdist,self.decay)
                    self.prior_std = self.prior_scale*self.prior_mean # default for now

so it seems to be making its own choices based on the current setting. However, note the use of self.bvec as an argument to self.bas.ixsc. self.bvec is not itself reset, as a result of which it will still have the old value.

That's the qualified good news. The bad news is that my earlier remark in point 7

I think we mostly are already using setter functions.

is basically false. The bifs class itself has almost no setters (set_prior_from_param_func comes close). bifs_gui does define setters for many of the parameters, which is what led to my initial remark.

The logic of keeping bifs objects internally consistent clearly belong in that class, not the gui.

[RossBoylan]

More issues with the same code. bifs.BIFS_MAP has this:

            if self.param_func_type != "Empirical":
                self.prior_std[np.where(self.prior_mean != 0.0)] = self.prior_scale*self.prior_mean[np.where(self.prior_mean != 0.0)]
                self.prior_std[np.where(self.prior_mean == 0.0)] = self.prior_scale*self.prior_mean_init
                self.prior_std2 = self.prior_std*self.prior_std

            # In case parameter function or decay value were reset:
            self.set_prior_from_param_func(self.param_func_type)

The problem is that set_prior_from_param_func may change the value of self.prior_mean and self.prior_std, in which case the calculations before the call should presumably be redone. As far as I can tell, they are not. It looks as if moving the call to set_prior_from_param_func before the steps that now precede it would fix the problem.