Geometry for circular detectors #1449

JevgenijaAksjonova · 2019-01-15T16:48:08Z

FanFlatGeometry was renamed to FanGeometry and modified. An additional parameter det_curve_radius was added to allow the usage of curved detectors. A new class CircularDetector was implemented. It can be viewed as re-implementation of existing class CircleSectionDetector with different input arguments and more readable formulas. Next, it is planned to implement new classes CylindricalDetector and SphericalDetector and modify the class ConeFlatGeometry to allow for the usage of curved detectors.

pep8speaks · 2019-01-15T16:48:12Z

Checking updated PR...

No PEP8 issues.

Comment last updated at 2019-03-06 14:04:07 UTC

adler-j · 2019-01-15T16:49:48Z

Could you have a look at the PEP8 failures?

See https://www.python.org/dev/peps/pep-0008/ for an overview of the official python style.

Most editors have a built in pep8 check, if yours doesnt you can use pytest-pep8 by running (in the odl folder):

pytest --pep8

I'll review after that's been cleaned up (so the review doesn't get removed by style fixes).

sbanert · 2019-01-15T19:06:33Z

There are still some issues remaining, @pep8speaks updates its post accordingly.

JevgenijaAksjonova · 2019-01-17T07:53:33Z

Thanks, I did not notice. These problem are not in my code though (except for the last one). Its about splitting the line before or after an operator, it appears that the standards have change, for instance see PyCQA/pycodestyle#513. Should I fix it so that it agrees with the new standards?

adler-j · 2019-01-17T07:57:37Z

PyCQA/pycodestyle#498 seems to be the latest one, I guess we should update according to this, could you help out with that @JevgenijaAksjonova ?

JevgenijaAksjonova · 2019-01-17T08:01:19Z

Sure

JevgenijaAksjonova · 2019-01-17T08:59:17Z

now it complains about the line break before =(

kohr-h

Finally I found the time to review this. Sorry for the delay.

The PR looks really good, and I've only got a bunch of minor comments.

odl/tomo/geometry/conebeam.py

odl/tomo/geometry/detector.py

JevgenijaAksjonova · 2019-02-07T14:48:40Z

Finally I found the time to review this. Sorry for the delay.

The PR looks really good, and I've only got a bunch of minor comments.

Thanks for the review! I have fixed most of the issues, however I have some questions/comments, which I will write next to each issue

odl/tomo/geometry/conebeam.py

adler-j · 2019-02-13T09:50:38Z

BTW @JevgenijaAksjonova you might want to have a look at e.g. https://docs.scipy.org/doc/numpy-1.15.0/dev/gitwash/development_workflow.html#writing-the-commit-message to get inspiration on how to write commit messages. It's not really a major worry but it helps keep the code organised.

odl/tomo/geometry/conebeam.py

kohr-h · 2019-02-13T10:49:00Z

odl/tomo/geometry/conebeam.py

+    True
+    >>> np.allclose(geom.det_axis_init, e_y)
+    True
+    """


@adler-j What do you think? Should we just drop this class and use FanBeamGeometry only? I'd be okay with that, given that this class doesn't really do anything.

Looking at this again, I think this class isn't needed any more. It shouldn't be lying around just as a name alias.

Can you remove it @JevgenijaAksjonova? That would mean to replace FanFlatGeometry with FanBeamGeometry everywhere, and removing the duplicate unit tests.

I am afraid that not all functionality that works for FanFlatGeometry works in general case, for instance FBP. In this case, I think we should live at as is for now.

That's true, but for FBP we could replace the isinstance(geometry, FanFlatGeometry) checks with isinstance(geometry, FanBeamGeometry) and geometry.det_radius is None.

Regarding the tests: I am not sure what to do with test_fanflat_props(shift) and test_fancurved_props(shift). There are some lines which are duplicates, but in general these are two different scenarios.

Well I guess can leave just the test_fancurved_props(), but call it test_fanbeam_props() instead

odl/tomo/geometry/detector.py

kohr-h · 2019-02-13T13:07:04Z

BTW @JevgenijaAksjonova you might want to have a look at e.g. https://docs.scipy.org/doc/numpy-1.15.0/dev/gitwash/development_workflow.html#writing-the-commit-message to get inspiration on how to write commit messages. It's not really a major worry but it helps keep the code organised.

Less of a worry now after #1471

JevgenijaAksjonova · 2019-02-25T12:09:29Z

So, are there any other changes necessary?

kohr-h · 2019-02-27T21:11:10Z

I've created a pull request into your branch that parametrizes the fan beam test to cover both cases of flat and curved detectors. See JevgenijaAksjonova#1
You can just merge it, and then we can go ahead with this.

kohr-h

Nice work! Now to resolve the conflict with master you need to rebase. That will also get rid of those PEP8 concerns.
After that I think we can go ahead and merge.

kohr-h · 2019-02-28T20:33:09Z

odl/tomo/backends/astra_cuda.py

@@ -372,7 +372,8 @@ def astra_cuda_bp_scaling_factor(proj_space, reco_space, geometry):
        if isinstance(geometry, Parallel2dGeometry):
            # Scales with 1 / cell_volume
            scaling_factor *= float(reco_space.cell_volume)
-        elif isinstance(geometry, FanFlatGeometry):
+        elif isinstance(geometry, FanBeamGeometry)\
+                and geometry.det_curve_radius is None:


Last little thing: Please use parentheses instead of backslash to continue the line.

kohr-h · 2019-02-28T20:33:18Z

odl/tomo/backends/astra_cuda.py

@@ -398,7 +399,8 @@ def astra_cuda_bp_scaling_factor(proj_space, reco_space, geometry):
        if isinstance(geometry, Parallel2dGeometry):
            # Scales with 1 / cell_volume
            scaling_factor *= float(reco_space.cell_volume)
-        elif isinstance(geometry, FanFlatGeometry):
+        elif isinstance(geometry, FanBeamGeometry)\
+                and geometry.det_curve_radius is None:


JevgenijaAksjonova · 2019-03-05T13:24:59Z

So, is everything OK? Can the code be merged?

kohr-h · 2019-03-06T13:45:26Z

odl/tomo/geometry/conebeam.py

@@ -307,6 +334,11 @@ def det_radius(self):
        """Detector circle radius of this geometry."""
        return self.__det_radius

+    @property
+    def det_curve_radius(self):


Last-minute comment, but part of API, thus important. I think it's better to use "curvature" instead of "curve" since the description of det_radius is described as "Detector circle radius", and this one as "Detector curve radius". That can be very confusing since a circle is a curve.

So please change det_curve_radius everywhere to det_curvature_radius. It is a bit long but much clearer, and also valid for 2D detectors.

After that I'll merge, I promise 🤞

kohr-h · 2019-03-06T14:35:54Z

Perfect, thanks a lot @JevgenijaAksjonova!

JevgenijaAksjonova · 2019-03-06T16:14:08Z

Great! Thank you for the code review!

adler-j self-requested a review January 15, 2019 16:51

adler-j added area: tomo status: review needed labels Jan 15, 2019

kohr-h requested changes Feb 4, 2019

View reviewed changes

kohr-h removed the status: review needed label Feb 4, 2019

kohr-h reviewed Feb 7, 2019

View reviewed changes

odl/tomo/geometry/conebeam.py Outdated Show resolved Hide resolved

kohr-h reviewed Feb 7, 2019

View reviewed changes

odl/tomo/geometry/conebeam.py Outdated Show resolved Hide resolved

kohr-h reviewed Feb 7, 2019

View reviewed changes

odl/tomo/geometry/conebeam.py Show resolved Hide resolved

kohr-h reviewed Feb 13, 2019

View reviewed changes

JevgenijaAksjonova force-pushed the master branch from c033887 to e658e3a Compare February 13, 2019 13:43

kohr-h approved these changes Feb 28, 2019

View reviewed changes

JevgenijaAksjonova added 6 commits March 1, 2019 09:26

Geometry for circular detectors

6307311

pep8 fixed

55ee0f8

more of pep8 fixed

1a5996e

CircularDetector issues fixed

82a4193

some more fixes

efab2ad

documentation + removing duplicate

da30b80

JevgenijaAksjonova and others added 8 commits March 1, 2019 09:34

Geometry for Circular detector

8cbea7d

passed doctests

1c7a347

changed an axample

4225f27

FanFlatGeometry removed

a920b3f

duplicate tests removed

18db02e

Parametrize fan beam test to cover flat and curved detector

8fc4e01

comment

3ea74bb

line breaks

9e82df6

JevgenijaAksjonova force-pushed the master branch from 6cbbcfb to 9e82df6 Compare March 1, 2019 08:35

JevgenijaAksjonova added 3 commits March 1, 2019 09:38

pep

fda7dab

one more instance of FanFlatGeometry found

dd637ae

one more instance of FanFlatGeometry found

95d2617

kohr-h reviewed Mar 6, 2019

View reviewed changes

renamed det_curve_radius to det_curvature_radius

a28c49d

kohr-h merged commit b8bb332 into odlgroup:master Mar 6, 2019

99991 mentioned this pull request Jan 27, 2023

odl issue adler-j/learned_primal_dual#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geometry for circular detectors #1449

Geometry for circular detectors #1449

JevgenijaAksjonova commented Jan 15, 2019

pep8speaks commented Jan 15, 2019 •

edited

Loading

adler-j commented Jan 15, 2019 •

edited

Loading

sbanert commented Jan 15, 2019

JevgenijaAksjonova commented Jan 17, 2019

adler-j commented Jan 17, 2019

JevgenijaAksjonova commented Jan 17, 2019

JevgenijaAksjonova commented Jan 17, 2019

kohr-h left a comment

JevgenijaAksjonova commented Feb 7, 2019

adler-j commented Feb 13, 2019

kohr-h Feb 13, 2019

kohr-h Feb 25, 2019

JevgenijaAksjonova Feb 26, 2019

kohr-h Feb 26, 2019

JevgenijaAksjonova Feb 27, 2019

JevgenijaAksjonova Feb 27, 2019

JevgenijaAksjonova Feb 27, 2019

kohr-h commented Feb 13, 2019

JevgenijaAksjonova commented Feb 25, 2019

kohr-h commented Feb 27, 2019 •

edited

Loading

kohr-h left a comment

kohr-h Feb 28, 2019

kohr-h Feb 28, 2019

JevgenijaAksjonova commented Mar 5, 2019

kohr-h Mar 6, 2019

kohr-h commented Mar 6, 2019

JevgenijaAksjonova commented Mar 6, 2019

Geometry for circular detectors #1449

Geometry for circular detectors #1449

Conversation

JevgenijaAksjonova commented Jan 15, 2019

pep8speaks commented Jan 15, 2019 • edited Loading

Comment last updated at 2019-03-06 14:04:07 UTC

adler-j commented Jan 15, 2019 • edited Loading

sbanert commented Jan 15, 2019

JevgenijaAksjonova commented Jan 17, 2019

adler-j commented Jan 17, 2019

JevgenijaAksjonova commented Jan 17, 2019

JevgenijaAksjonova commented Jan 17, 2019

kohr-h left a comment

Choose a reason for hiding this comment

JevgenijaAksjonova commented Feb 7, 2019

adler-j commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h commented Feb 13, 2019

JevgenijaAksjonova commented Feb 25, 2019

kohr-h commented Feb 27, 2019 • edited Loading

kohr-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JevgenijaAksjonova commented Mar 5, 2019

Choose a reason for hiding this comment

kohr-h commented Mar 6, 2019

JevgenijaAksjonova commented Mar 6, 2019

pep8speaks commented Jan 15, 2019 •

edited

Loading

adler-j commented Jan 15, 2019 •

edited

Loading

kohr-h commented Feb 27, 2019 •

edited

Loading