Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of h_center parameter in linop.CircularConvolve #299

Merged
merged 3 commits into from
May 17, 2022

Conversation

bwohlberg
Copy link
Collaborator

Improve handling of h_center parameter in linop.CircularConvolve. Allow tuples and lists instead of just DeviceArray type, as well as float or int for one-dimensional arrays. Also add tests for this parameter.

@bwohlberg bwohlberg added the improvement Improvement of existing code, including addressing of omissions or inconsistencies label May 16, 2022
@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #299 (fd47de8) into main (9d1fa8e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #299   +/-   ##
=======================================
  Coverage   94.30%   94.31%           
=======================================
  Files          53       53           
  Lines        3321     3326    +5     
=======================================
+ Hits         3132     3137    +5     
  Misses        189      189           
Flag Coverage Δ
unittests 94.31% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/linop/_circconv.py 84.61% <100.00%> (+0.77%) ⬆️

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 1d1154b...fd47de8. Read the comment docs.

else:
# support float or int values for h_center
if isinstance(self.h_center, (float, int)):
offset = -snp.array(
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting looks slightly odd here but I assume that's black's doing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the relatively deeply nested if/else structure? That's just a natural consequence of the actual logic here.

@bwohlberg bwohlberg merged commit 039489b into main May 17, 2022
@bwohlberg bwohlberg deleted the brendt/circconv branch May 17, 2022 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of existing code, including addressing of omissions or inconsistencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants