Replace src.filters array with something more explicit

[janden]

Right now, we have an array src.filters that lists all the CTF filters for the images in an ImageSource. In reality, there are only a small number of filters in a dataset, so there's a natural mapping from the set of images to the (much smaller) set of images. Right now, we deal with this by constructing the array so that it contains many references to a small set of filters, but this causes problems, notably here:

ASPIRE-Python/src/aspire/source/__init__.py

Line 363 in 60f4ced

f.__class__ = MultiplicativeFilter

and here:

ASPIRE-Python/src/aspire/estimation/covar2d.py

Line 350 in 60f4ced

unique_filters = list(set(src.filters))

A better approach might be to keep two separate variables: filters and filters_idx, the latter of which contains the indices corresponding to the list of unique filters in filters.

[vineetbansal]

A related issue we found today is that the Simulation class needs to initialize its __filter_indices metadata column properly, which will allow us to support this for simulations.

self.set_metadata('__filter_indices', <values>)

[junchaoxia]

Took a look again for this issue. I am summarizing the things need to be done as below:

1. In the ImageSource class, create two public variables of filters and filters_idx for storing CTF filters types and indices to the types. The original src.filters should be removed.
2. Update the functions for assigning the filters and filters_idx in the Relion and Simulation classes. The interactions with meta_data and star file need to be taken care of too.
3. Update related functions in Filters, Cov2D and other modules.

After finishing above, we can continue to work on issue #45 and #279.

[vineetbansal]

(1) and (2) above are not necessary, as the infrastructure for these is already set up. We discussed this in detail. Also I don't understand why you say src.filters should be removed - addition to a filter_idx (which is unnecessary anyway) doesn't imply that you won't need src.filters.

I'm pasting the points that we went through in our code walkthrough. Each of these should be a commit by itself. The first 2 points should constitute one PR, as should (3), and then (4) and (5)

PR 1

    - Add filter indexes for simulation classes (populate the __filter_indices metadata column).
    - Use the filter index instead of set(self.filters) in the 2 places (as the original issue points out). Don't worry about changing the `__class__` stuff etc. at this point or whether you're modifying a filter in place.
    - Get rid of the __filter metadata column entirely, and the getters/setters that access it.
    - Add a setter for the filter indices property. Make sure to add logic for changing defocus_u/defocus_v etc. that is currently in the filters setter.
    - Make sure that all access to filter indices is through the property getter/setter
    - If you do have to access any metadata column, do it through `get_metadata` and `set_metadata`, not directly.

PR 2

    - Add a couple of tests for checking the actual values of filter output, before and after using 'scale'.

PR 3

    - Modify Filter object to return new filter instead of modifying in place.
    - Modify the tests to use this functional approach (without changing the values in the unit tests), and modify the code in 2 places (identified in step 2) to do filter re-assignment instead of modifying `__class__` etc.

Finally, we don't need source.filter_idx property (since this is accessible through the getter), and source.filters_typ should be renamed to something clearer like source.unique_filters.

[junchaoxia]

@vineetbansal Thank for listing the points again. I think the focus we discussed is making the filter immutable related to issue #279. Here, my understanding is that, @janden wants to replace self.filters (which are ctf filters fro all images) with two parameters self.filters_tpy (only saving the unique ctf filters) and self.filters_idx (integers for all images to represent the indices of in unique filters). So in the way unique_filters = list(set(src.filters)) and f in unique_filters are not necessary, because we can use for f_ind in range(len(self.filters_typ)) to go through each unique filters. I believe self.filters_tpy and self.filters_idx can be built from self._metadata['__filter'] and self._metadata['__filter_indices'] of Relion class. For Simulation class, they need to be passed in in the constructor. By the way, the issue #45 can be solved by passing the self.filters_idx in the constructor. If we do this, the unique_filters = list(set(src.filters)) in other classes such as Cov2d need to be replaced.

I think after we make the replacements, we can focus on the immutable problem using the last two steps listed by @vineetbansal.

[vineetbansal]

I would say if @janden wants to replace self.filters (a list of pointers to a handful of filters), with a list of unique filter objects, do it after all the points I mention above, as a separate PR. Trying to do this all at once will be tricky.

[junchaoxia]

@vineetbansal I am almost done for the issue of replace self.filters with self.filters_typ and self.filters_idx. There are quite lots of changes. I might push back tomorrow morning. After that I will make the changes to immutable issue.

[janden]

I'm having trouble following this discussion (there seems to have been another discussion between the two of you that I'm not familiar with). We do indeed want to replace filters with a list of unique filter objects coupled with an array of indices. There's some other stuff to be done as well, and I agree that these should probably be done as separate PRs.

[junchaoxia]

The related issues have been addressed in PRs #295 and #309. Closing now.