mutable or immutable of ImageSource Class

[junchaoxia]

The original design of ImageSource class targets an immutable class which prevents the change of class variables and metadata. However, this is not exactly followed such as in the downsample function that resets the class variable L for the image size due to the downsampling and in whitening function that modifies the type of CTF filters. The reset of L can cause crash in the Simulation class when the clear images need to be reconstructed at different stages. We have a few options need to be discussed.

1. Use additional variable such as _L for the original image size.
2. Implement the clone method to generate new object when changing values of variables is needed.
3. Derive new ImageSource class for each submodule or preprocess step such as the DenoisedImageSource class.

[garrettwrong]

Cool, thanks for making this. Tagging @vineetbansal who will join us (next month) to chat about this.

[vineetbansal]

I agree that self.L should not be changed by downsample.

The reasons that I can think of as to why it's currently doing so:

• The self.generation_pipeline object (where we should be appending the DownSample transform, as we're also doing right now), may not be able to handle Transforms of different resolutions (I may be wrong about this).
• The expectation by the caller that downsample will downsample everything about image generation (including any filters/shifts etc). This may not have been possible to do in an earlier iteration of the code, but with the Pipeline object it should be straightforward. Even if it works correctly in terms of generating the final images though, there's the question of whether the metadata associated with the the ImageSource should change, or should not, which we should discuss with Joakim.
• The Simulation class stores all it's attributes but doesn't consult them till we ask for _images(), similar to the RelionSource class. The current behavior of changing self.L may simply have been a clumsy attempt to store the intention of downsampling, with the expectation that it would not be used till later. Again, with the Pipeline object, the _images() method doesn't need to know that there's a downsampling coming down the line, and it could work with the (unchanged) self.L.

I also strongly suggest that we disable the cache() method in ImageSource to avoid any interactions. The Pipeline object is designed to cache images if necessary, but we can enable that once things work correctly.

It may sound like I'm furiously defending Pipeline. I'm not - even if the implementation is changed, I think it's the correct abstraction to think about these these operations.

For (2):
Implement the clone method to generate new object when changing values of variables is needed.

I think a shallow copy should suffice, as long as all attributes are captured by __init__, so we may not have to implement anything specific to this class.

I don't quite understand (3):
Derive new ImageSource class for each submodule or preprocess step such as the DenoisedImageSource class.

One person's denoised image would be another's noisy image, so capturing past operations in a class structure doesn't make sense to me, but I might be misunderstanding something here.

[janden]

Thank you for these comments, @vineetbansal. I think your second reason for why self.L is modified by downsample sounds the most likely (that we expect downsample, or any other preprocessing function, will modify the metadata as needed). The issue here is that Simulation is using these external variables for internal purposes (this split also exists for shifts, rots, filters, and so on, that are used to generate the images, but may be different from the ones exposed to the outside). We should try to separate these things as much as possible.

I'm not sure disabling cache is a good idea. It allows the user to explicitly control what's being cached and when. We can disable it temporarily, but this functionality needs to exist at some point. Pushing it into Pipeline carries the risk of obscuring the cache mechanism and making it harder for the user to interact with.

Regarding a new ImageSource class for each operation, what (I think) @junchaoxia is referring to is a functional model for sources (more like the structure of the Matlab code). In other words, instead of stacking a set of operations to be performed later (and dealing with the attendant metadata headaches). This way, a Simulation wouldn't have to worry about what variables are internal and external, both are identical. Then, when we apply some image processing operation (like downsample), this would result in a new ImageSource object (either through a different class or some other mechanism) that serves up the downsampled images (by calling the images method of its parent Simulation and applying the necessary downsampling) with the modified metadata. Both original Simulation object and this downsampled ImageSource would exist at the same time, the latter would refer to the former, but there would be no need to keep a list of transformations or worry about which metadata variables belong to the Simulation (internal) or to the downsampled ImageSource external.

[junchaoxia]

Thanks for @vineetbansal and @janden. I believe the design of cache is due to some cases that we don't want to regenerate all original images through the _images() function of derived ImageSource classes such as Simulation and RelionImageSource. But the current cache implementation only works when we can load all images into memory. The cache function also allows us to replace the original images with an external Image object with modified images. The design of pipeline mainly allows us to accumulate a series of operations on the images and only apply them on the original images (_im) when the images() is called. As discussed above the original design of ImageSource is an immutable object and we also created the ArrayImageSource object to hold the images in memory.
During the development of Cov2d module, @janden and I came out the way of derived class DenoisedImageSource for operations on denoised images, where _image() overrides the base one to call denoising operation and generate the denoised images. This is a very similar way as the derived Simulation and RelionImageSource classes but we have new object for changed variables and metadata. This is an example of the third way listed above. In general all the preprocess steps can be done in this way so that the Simulation and RelionImageSource classes only build the original images and no variables and metadata are changed.

[garrettwrong]

Adding a brief from our call today (Junchao, Vineet, Garrett present).

• Confirmed Vineet was able to build run and pass unit tests from develop.
• Reviewed some of the updates relating to this issue for context.
• Demonstrated this issue with Vineet. With some clarification, we were able to reproduce the issue with Vineet.
• We discussed some of the approaches mentioned above.
• Vineet will try to focus on where exactly the resolution breaks down the subsequent pipeline steps, and may consider a potential design that will admit looping over stages in the pipeline.
• We will plan to follow up at the Aug 10 3pm meeting.

I think that is most of it, I regret not writing this earlier :). Please add anything missed.

[garrettwrong]

I was going through issues this morning and noticed this may be related to #46 . If that is the case we should remember to close that one out once this is resolved.

[junchaoxia]

Recently developments has tended to derived new ImageSource classes for saving denoised images and orientation angles. Hence the ImageSource class should be treated as immutable. Closing now.

[janden]

Recently developments has tended to derived new ImageSource classes for saving denoised images and orientation angles. Hence the ImageSource class should be treated as immutable. Closing now.

No the preprocessing functions still modify the object.