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

Add support for 3D tomographic projection with astra #427

Merged
merged 17 commits into from
Jul 11, 2023

Conversation

Michael-T-McCann
Copy link
Contributor

Supersedes #424

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #427 (8bf2a74) into main (523d662) will decrease coverage by 0.19%.
The diff coverage is 78.12%.

@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
- Coverage   94.82%   94.63%   -0.19%     
==========================================
  Files          86       86              
  Lines        5422     5471      +49     
==========================================
+ Hits         5141     5177      +36     
- Misses        281      294      +13     
Flag Coverage Δ
unittests 94.63% <78.12%> (-0.19%) ⬇️

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

Impacted Files Coverage Δ
scico/linop/radon_astra.py 79.65% <74.07%> (-6.84%) ⬇️
scico/examples.py 100.00% <100.00%> (ø)

@Michael-T-McCann Michael-T-McCann marked this pull request as ready for review June 16, 2023 23:12
@bwohlberg bwohlberg mentioned this pull request Jun 16, 2023
examples/scriptcheck.sh Outdated Show resolved Hide resolved
examples/scriptcheck.sh Outdated Show resolved Hide resolved
@bwohlberg bwohlberg added the enhancement New feature or request label Jun 16, 2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

The new example has been added to the main index file, but I don't see it in the built docs. Perhaps you still need to run makeindex.py and commit the changed secondary index files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I did this, but it's still not showing up as far as I can tell. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a problem with the submodule commit attached to this branch. When the submodule changes in a PR, it's usually cleanest to make a corresponding branch and PR for scico-data, with the submodule here linked to the head of that branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, the generated notebook was blank and sphinx was smart enough to not include it. Working on a fix.

@bwohlberg
Copy link
Collaborator

It would be worth adding a note on this extended capability in CHANGES.rst.

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.

Looks good subject to some minor corrections.

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.

Looks good except for the absence of the new example notebook in the docs. This appears to be because the notebook corresponding to the new example script is bad: it's just an almost-empty .ipynb structure, with no real content. Rebuilding the notebook should fix this issue, I would hope. The previous build probably failed because it was run on a host without a GPU.

@Michael-T-McCann Michael-T-McCann merged commit a07809c into main Jul 11, 2023
@Michael-T-McCann Michael-T-McCann deleted the mike/astra_3d branch July 11, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants