Improve X-ray docs #578
Improve X-ray docs #578
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #578 +/- ##
==========================================
- Coverage 93.55% 93.49% -0.06%
==========================================
Files 92 92
Lines 6182 6186 +4
==========================================
Hits 5783 5783
- Misses 399 403 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scico/linop/xray/astra.py
Outdated
| @@ -604,12 +620,15 @@ def __init__( | |||
|
|
|||
| if not isinstance(det_count, (list, tuple)) or len(det_count) != 2: | |||
| raise ValueError("Expected det_count to be a tuple with 2 elements.") | |||
| if angles is not None and vectors is not None: | |||
| raise ValueError("`angles` and `vectors` are mutually exclusive.") | |||
There was a problem hiding this comment.
Perhaps "Parameters angles and vectors ..."? Also, exceptions typically go to a text console, so no point in adding markup. (Also applies to other exceptions in this PR.)
There was a problem hiding this comment.
Understood about error messages going to the console, but I do think it's useful to have some marker that we are talking about a parameter name. E.g., numpy uses single quotes.
sum() missing 1 required positional argument: 'a'
There was a problem hiding this comment.
Fair point. But for consistency we should make a note (perhaps a new issue) to also apply that principle elsewhere in the code.
There was a problem hiding this comment.
Issue #583 created.
scico/linop/xray/astra.py
Outdated
| @@ -65,13 +65,18 @@ def _project_coords( | |||
| x_volume: np.ndarray, vol_geom: VolumeGeometry, proj_geom: ProjectionGeometry | |||
| ) -> np.ndarray: | |||
| """ | |||
| Transform volume (logical) coordinates into world coordinates based | |||
| Project volume (logical) coordinates into detector coordinates based | |||
There was a problem hiding this comment.
Is this a change of terminology from "world coordinates" to "detector coordinates"? If so, corresponding changes are required elsewhere for consistency.
Why are volume coordinates "logical"?
There was a problem hiding this comment.
Not a change in terminology, the previous docstring was incorrect.
Re volume vs logical: I hoped "logical" communicates that they are like integer indexes, but I'm removing it because I do not think it helps.
There was a problem hiding this comment.
It looks as if "logical" is still present.
There was a problem hiding this comment.
It would also be good to mention somewhere what "world" coordinates mean.
There was a problem hiding this comment.
Added to the module docstring after all.
scico/linop/xray/astra.py
Outdated
| @@ -95,7 +100,10 @@ def project_world_coordinates( | |||
| """Project world coordinates along ray into the specified basis. | |||
|
|
|||
| Project world coordinates along `ray` into the basis described by `u` | |||
| and `v` with center `d`. | |||
| and `v` with center `d`. The term ""world"" emphasizes that the | |||
There was a problem hiding this comment.
Why double quotes on either side of "world"?
There was a problem hiding this comment.
I want that to print as
The term "world" emphasizes...
using the quotes to indicate I'm talking about a word as word. I think the double double quotes escapes the double quotes to achieve that.
There was a problem hiding this comment.
I couldn't confirm this since the function in question isn't included in the rendered API docs, but it would be worth checking that this syntax usage is correct in case this function ever does get included in the docs.
There was a problem hiding this comment.
This is no longer an issue given that I moved the discussion to the module docstring and rephrased.
The discussion might work better in the module docstring at the beginning of the file. |
No comment? |
Added a discussion of this to the module docstring. |
Addresses #576. Is there a better place to put the text about world coordinates?