Standardize evaluate and evaluate_t behavior in bases
#618
Conversation
Merge pull request #584 from ComputationalCryoEM/develop
BFSReddyChetterji k Bug
…imators (#550) * changed self.L and self.n to self.src.L and self.src.n for estimators * extraneous comma
…o second-level API access (#565) * utils.coor_trans methods accessible from second level * crop_2d method * crop2d test * clarify flooring behavior for centering and add apdding * tests finished * isort shenanigans * fix even to odd n->n+1 padding bug * slightly better comment * better behavior for dtype * remove mistaken characters * fixes after merge * last ensure removed * rect tests * padding tests and fill value test * better comments * even better comments * np.diag * crop_pad_2d * clarifying comment when padding
* downsample tests * cleanup and deleting test downsample from preprocess_pipeline * remove hardcoded ds / whiten test * flake8 * 3d volume downsample test * remove redundant test from tests/test_preprocess.py * remove unused import * remove numbered tests for uniformity * typo * skip one of the ds tests * use gaussian_3d to generate test volume * isort * checkCenterPoint * max_resolution -> L_ds * parametrize ds tests
* docstring for besselj_zeros * rest of docstrings * remove pdb import * second order differences comment * comments and docstring for num_besselj_zeros * formatting * tiny fixes; * docstring fixes * NumPy * NumPy again * sanity check * parentheses inside sentence
* fix size checking bug * fb2d * fb3d and pswf2d * dirac * fpswf * polar * format * sz -> size * test init with int * docstring * typo * the same typo
Cast `Images` to arrays when needed.
|
Summary of changes: Testing inputs and outputs of
Most type checks and dtype warnings moved up to |
j-c-c
left a comment
j-c-c
left a comment
There was a problem hiding this comment.
This looks good. The mixin definitely streamlines the tests. Just a couple small comments.
janden
left a comment
janden
left a comment
There was a problem hiding this comment.
Just did a high-level overview to begin with so that we can iron out the details.
| f"{self.__class__.__name__}::evaluate_t" | ||
| f" passed numpy array instead of {_class}." | ||
| ) | ||
| v = _class(v) |
There was a problem hiding this comment.
They way I read the intent of the PR was that the inner methods (_evaluate) and (_evaluate_t) should only deal with ndarrays. Why are we casting here?
There was a problem hiding this comment.
The evaluate_t methods for the various bases are a bit inconsistent as far as whether they expect Image/Volume or arrays. The choice was many modifications to all the specific functions versus just a few and passing an Image/Volume. I didn't initially intend to dig super deep into each individual method with this PR, just impose the overall structure and enforce the return / input types.
Do you think it would be better to make all those changes in this PR?
chris-langfield
force-pushed
the
basis_returns_553
branch
from
26a6a3d to
c01a5e3
Compare
Closes #553
Enforcing the rule that
evaluateshould return an Image/Volume andevaluate_tshould return a NumPy array means a few small changes are needed around the codebase.#553 (comment)