Skip to content

Conversation

@chris-langfield
Copy link
Collaborator

Address #38

I'm not sure if this method is unnecessary. The code in eval_filter_grid does some manipulation with the filter_indices of the source that is only applicable within that class.

It is only used in two implementations of the Estimator.compute_kernel method: in aspire.reconstruction.mean and aspire.covariance.covar

@chris-langfield chris-langfield linked an issue Nov 11, 2021 that may be closed by this pull request
@chris-langfield chris-langfield self-assigned this Nov 11, 2021
@chris-langfield chris-langfield changed the base branch from master to develop November 11, 2021 16:39
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #517 (d7ccd1a) into develop (b146d33) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #517   +/-   ##
========================================
  Coverage    86.54%   86.54%           
========================================
  Files          106      106           
  Lines         7660     7660           
========================================
  Hits          6629     6629           
  Misses        1031     1031           
Impacted Files Coverage Δ
src/aspire/operators/__init__.py 100.00% <ø> (ø)
src/aspire/source/image.py 95.40% <ø> (-0.21%) ⬇️
src/aspire/covariance/covar.py 88.79% <100.00%> (+0.09%) ⬆️
src/aspire/operators/filters.py 96.33% <100.00%> (+0.22%) ⬆️
src/aspire/reconstruction/mean.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b146d33...d7ccd1a. Read the comment docs.

@chris-langfield
Copy link
Collaborator Author

As discussed earlier it seems like the best solution here is to simply move the whole method into operators.filters as a module method (outside of the Filter class).

Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, one todo, otherwise lgtm. Hit those and should be ready to undraft. Thanks

remove method from ImageSource

fix imports

no self

import cleanup

docstring

clarity and style fixes
@chris-langfield
Copy link
Collaborator Author

Rebased / squash prior to undraft

garrettwrong
garrettwrong previously approved these changes Dec 7, 2021
@chris-langfield chris-langfield marked this pull request as ready for review December 7, 2021 16:12
@chris-langfield chris-langfield linked an issue Dec 9, 2021 that may be closed by this pull request
@chris-langfield chris-langfield merged commit 82692ea into develop Dec 17, 2021
@chris-langfield chris-langfield deleted the evaluate_grid_i38 branch December 17, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unnecessary method in ImageSource

4 participants