Filter stack and radial covar optimizations#1388
Conversation
garrettwrong
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1388 +/- ##
===========================================
+ Coverage 90.56% 90.65% +0.08%
===========================================
Files 135 135
Lines 14691 14926 +235
===========================================
+ Hits 13305 13531 +226
- Misses 1386 1395 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1102a7e to
5b5ea36
Compare
[skip ci]
warning sysetm needs improving
still needs cleanup
d141fea to
6c11b8f
Compare
4b85c73 to
8b9beee
Compare
e062964 to
8d9e1ae
Compare
|
Going to kick off the full CI suite. |
janden
left a comment
janden
left a comment
There was a problem hiding this comment.
Nice work! A few questions here and there, but nothing major.
| """ | ||
| Expands radial vector or stack of vetors `radial_vec` to basis matrix. | ||
|
|
||
| :param radial_vec: Array holding radial vector, |
There was a problem hiding this comment.
But how do we know whether these radial pts are compatible with the radial grid used here (from _precomp["gl_nodes"])?
There was a problem hiding this comment.
For the radial case, gl_nodes did not work well as-is, I had to remake them in double precision :/
# Weights appear a little sensitive to dtype, otherwise could use self._precomp["gl_nodes"]
I changed the occurrence of 0.5 to self.kcut in (one new, one existing)lgwt calls. kcut is fixed to 0.5. That would more/less enforce self consistency within the basis.
As far as the input to the function... ASPIRE code is generating the radial vectors by using basis._filter_pts. If that attribute doesn't exist the code I wrote to generate the radial vector will not succeed and the code will not reach this point. Now, If a user is cooking their own radial vector using their own code, that is their responsibility. They can read the code.
There was a problem hiding this comment.
I've added
See `_filter_pts` for point set.
to both expand_radial_vec docstrings.
| for _ in radial_vec | ||
| ] | ||
|
|
||
| ind_ell = 0 |
There was a problem hiding this comment.
A lot of the work here seems to be doubling up what's done in filter_stack_to_basis_mats. Would refactoring be an option here?
There was a problem hiding this comment.
Yes, it is. The first half of those methods are totally different, but they are similar after that. I'll try seeing what calling expand_radial_vec looks like after the averaging to a 1d vector in _filter_stack_to_basis_mats.
There was a problem hiding this comment.
Yeah, that seemed to work out fine. I'll try similar for FLE.
There was a problem hiding this comment.
Okay. So I was able to make both FFB and FLE call their own expand_radial_vec right after their 2d to 1d averaging. I had to modify the FLE functions a little to do this, as their expand_radial_vec was doing some additional Fourier padding by default.
| ) | ||
|
|
||
| def filter(self, filter): | ||
| def convolve(self, filter_values): |
There was a problem hiding this comment.
Having both filter and convolve can be confusing since they basically describe the same operation. How about calling this one _filter and calling it from filter as done below?
| :param filter: An object of type `Filter`. | ||
| :return: A new filtered `Image` object. | ||
| """ | ||
| # Note image and filter data is intentionally migrated via |
There was a problem hiding this comment.
I think this comment is stale. The xp.asarray migration happens above in convolve. The last sentence is also missing parts.
There was a problem hiding this comment.
yeah, my bad, that was sloppy on my part. I must have got distracted. Fixed.
| """ | ||
| return 1 | ||
|
|
||
| def _ctf_params(self): |
There was a problem hiding this comment.
Are these still needed? I don't recall seeing them used anywhere.
There was a problem hiding this comment.
I originally wrote it as a bridge (before the filter stack conversion). I left it for three minor reasons.
It ended up making a call in Simulation cleaner/simpler. filter_stack_params = self.filter_stack._ctf_params()[ :, :6 ] replaces a lookup loop over starfile mapping/attributes before the data is being repacked from per-filter to per-image for the set-metadata call. (The mapping still exists, its just implicit in the structure now.) Ie, it moves the lookup/packing of the filter attributes from Simulation to Filter code, which is Filter business imo. Then the only thing that happens in Simulation is the ctf to image mapping, which is Simulation business.
If anyone ever "just wants all the ctf params" It's way more convenient one-liner than dropping the starfile (gross) or duplicating code that teases them out one-by-one (ie someone copying the loop code out of from Simulation, which I've had to do several times now. For example...).
I use it to add tests for equality/inequality of {Radial}CTFFilters which is useful for debugging.
I can revert it if non of those seem worth having the code change.
|
|
||
| return CTFFilter( | ||
| self.voltage[items], | ||
| # self.wavelength[items], |
There was a problem hiding this comment.
sure! missed that.
| # Additionally we upcast so downstream computations remain in doubles. | ||
|
|
||
| # First prepare arrays for broadcasting. | ||
| voltage = xp.atleast_1d(voltage)[:, None] |
There was a problem hiding this comment.
Shouldn't this be np since we haven't moved over to GPU yet?
There was a problem hiding this comment.
In the PR I added an agnostic atleast_1d that will return the type of array (np/cp) it is given. If we're in xp as cupy mode, that will be the one used here.
The way it is written, ctf_formula should accept the inputs on host, gpu, or a mix and probably not crash.
Reverting would limit the param inputs to host only. I'd leave it.
|
@janden for the |
2 participants
Converts code towards filter stacks while maintaining ability to use legacy single filters (for now).
Provides a
to_radialmethod for Filters.Provides means to mix most singleton and stack filters. Vectorizes bulk evaluations.
Tries to vectorize (and sometimes use GPU when things fit) for
filter_statck_to_basis_mats.Add code paths to run radially optimized
filter_stack_to_basis_mat.Adds a smoke/timing test for covar2d using 10028 (ribsome) and 11618 (ctf per image) experimental sized cases.
Still a WIP, need self review.