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.

Sign up for GitHub

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

Jump to bottom

Add Abel linear operator #208

Merged
merged 57 commits into from
Feb 8, 2022
Merged

Add Abel linear operator #208

merged 57 commits into from
Feb 8, 2022

Conversation

smajee
Copy link
Contributor

@smajee smajee commented Feb 2, 2022 •
edited by bwohlberg
Loading

Add Abel linear operator.

smajee and others added 22 commits December 19, 2021 11:54
@smajee
Add forked pyabel in requirements
8cb01e8
@smajee
add abel transform code and rough example
88030ab
@smajee
add inverse function
9813eb0
@smajee
add noise in example
81b5dca
@smajee
remove old example
2a4cf11
@smajee
Update example to use on-the-fly generated phantom
bb330ba
@smajee
change abel interface to use low llevel functions of pyabel
3089f6b
@smajee
misc changes to accomodate jaxification
acc3ba5
@bwohlberg
New approach to jax-ifying pyabel functions
e84f92a
@bwohlberg
Minor cleanup
2d8770e
@bwohlberg
Merge branch 'main' into brendt/abel
e017957
@bwohlberg
Update pyabel dependency
8e130a5
@bwohlberg
Merge branch 'main' into brendt/abel
786a324
@smajee
remove unused center argument
7ee150b
@smajee
fix display scaling in example
8a51249
@smajee
deumpyfy
313d1ec
@smajee
initial tests
e47c366
@smajee
fix merge conflicts
735d7a3
@smajee
update tests
d5f7b9c
@smajee
add variations of test
5007728
@smajee
add tests
1d7d7d5
@smajee
Merge branch 'main' into smajee/abel
88ba9cc
@bwohlberg bwohlberg added the enhancement New feature or request label Feb 3, 2022
bwohlberg
bwohlberg reviewed Feb 4, 2022
View reviewed changes
Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

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

Everything looks good except for one remaining issue that I only just noticed: many of the functions and methods in scico/linop/abel.py don't have type annotations.

Michael-T-McCann
Michael-T-McCann reviewed Feb 7, 2022
View reviewed changes
Copy link
Contributor

@Michael-T-McCann Michael-T-McCann left a comment

Choose a reason for hiding this comment

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

Nice work!

examples/scripts/ct_abel_tv_admm.py Outdated Show resolved Hide resolved
scico/linop/abel.py Show resolved Hide resolved
scico/test/linop/test_abel.py Show resolved Hide resolved
crstngc
crstngc reviewed Feb 7, 2022
View reviewed changes
Copy link
Contributor

@crstngc crstngc left a comment

Choose a reason for hiding this comment

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

Looks good to me, bar some comments. Perhaps also add a test for inverse function?

scico/linop/abel.py Show resolved Hide resolved
scico/linop/abel.py Outdated Show resolved Hide resolved
scico/linop/abel.py
Args:
img_shape: Shape of the input image.
"""
self.proj_mat_quad = _pyabel_daun_get_proj_matrix(img_shape)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this (self.proj_mat_quad) have type annotation?

Copy link
Contributor Author

@smajee smajee Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The _pyabel_daun_get_proj_matrix function has an output type annotation. Did you mean adding it in the body of the __init__ function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I do not know how strict are we supposed to be with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now the primary requirement is type annotations for functions and methods. Once we have type checking working (see #176) we can start worrying about typing annotations for attributes/variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, shall we postpone this to a later time when #176 is done?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

@smajee
Copy link
Contributor Author

smajee commented Feb 8, 2022

Looks good to me, bar some comments. Perhaps also add a test for inverse function?

Added tests for the inverse function

@bwohlberg bwohlberg merged commit 8808b82 into main Feb 8, 2022
@bwohlberg bwohlberg deleted the smajee/abel branch February 8, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crstngc crstngc crstngc left review comments

@Michael-T-McCann Michael-T-McCann Michael-T-McCann approved these changes

@bwohlberg bwohlberg bwohlberg approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@smajee @bwohlberg @crstngc @Michael-T-McCann