AdvancedPhotonSource / xdesign

Tools for designing x-ray phantoms and experiments.
https://xdesign.readthedocs.io
Other
24 stars 16 forks source link

ENH: Add 3D geometry and propagation module #50

Closed mdw771 closed 7 years ago

mdw771 commented 7 years ago

Geometry:

Material:

Plot:

Acquisition:

Demo codes:

Other changes:

carterbox commented 7 years ago

@mdw771, Please fix the error that cause failing Travis builds. It looks like you forgot to remove the import statements for the grid module. There is also an import error for skimage.morphology probably because skimage isn't listed in the requirements.txt or in the install section of travis.yml.

We are targeting both python2.7 and python3.x, so you need to use parenthesis on print statements.

Thanks for your patience in advance. I'm going on vacation next week, but I'll try and review as much as I can today.

mdw771 commented 7 years ago

Thanks for the comments.

carterbox commented 7 years ago

I suggest that the discrete functions takes only the number of pixels per side for the grid and simply assume the real-size of each pixel to be 1, since are not important unless physics calculations (eg. propagation simulation) is involved.

I don't understand. Why is the real size of each pixel not important? Every Entity has a real size in centimeters, so how is the real size of each pixel not important?

Also, all the TravisCI tests need to pass before this pull is approved.

mdw771 commented 7 years ago

I suggest that the discrete functions takes only the number of pixels per side for the grid and simply assume the real-size of each pixel to be 1, since are not important unless physics calculations (eg. propagation simulation) is involved.

I just realized that the point coordinates are supposed to be given in cm instead of in pixels. Now this makes more sense to me. But I still suggest to change the current scheme that fixes the entire sample size at 1 cm to a scheme that requires inputs of individual pixel size and number of pixels (grid size). When the model is used for microtomography or holographical studies people are generally interested in resolution of micron or nanometer level. It is necessary in such cases to let users specify the pixel size in order to limit the grid size.

carterbox commented 7 years ago

people are generally interested in resolution of micron or nanometer level

What I think you mean is: having a 1 cm field of view is too large for some experiments; the ability to specify a smaller region for plotting or discretization is good.

I agree :smile:. I think the best way to handle this is to have two keyword arguments such as below for the discretization function.

"""
psize : float
    The side length of a cubic pixel.
bounding_box : :py:class:`numpy.ndarray`, :py:class:`List`
    The desired field of view specified as `[min, max]` where min and max
    are two column vectors pointing to the min and max corners of the
    desired field of view.
"""

bounding_box would default to [[0, 1], [0, 1], [0, 1] ...].

Does that sound good?

mdw771 commented 7 years ago

I think the best way to handle this is to have two keyword arguments such as below for the discretization function.

This would address the problem, but I'm thinking of another way which seems more straightforward: we set 3 keywords arguments:

"""
psize : float
    Side length of a pixel or voxel in cm.
grid_length : float
    Length of a side of the grid in cm (this is the quantity that was originally
    default to 1 cm).
grid_size : int
    Number of pixels along a side of the grid (this is the original "size" argument).
"""

All arguments default to None and user shall supply any 2 of them for the function to infer the other one. In this way the grid dimension is not necessarily 1 cm, but the function would be more flexible for various situations. How do you think of this?

carterbox commented 7 years ago

I have two arguments for the 2 arg API over the 2 of 3 arg API.

  1. The advantage of specifying the bounding_box is that you can reduce computation by avoiding empty regions. grid_length and grid_size and psize, tells how many pixels are wanted but not where they are wanted, so the problem is under-defined. If you assume the grid includes the origin, this could be expensive if your object of interest is very small and far away from the origin.

  2. I oppose the "choose only two of three optional arguments". I tried that with another API, and it made the code messy. You have to check user inputs to make sure only 2 of the 3 choices were supplied, and you have to convert the input to the parameters that the implementation actually uses.

mdw771 commented 7 years ago

Travis test of test_tube_particles.test_model_prop_pipeline fails on server due to the absence of xraylib. After adding xraylib in requirements.txt and inserting conda install xraylib in .travis.yml, the building process seems stuck and went timeout after 10 min. Local tests all went through with Python 2.7.13 (Anaconda) except a Not equal to tolerance rtol=1e-07, atol=0.01 failure in tests.test_3d.test_probe_circular.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling f8baf6db5e95ade3a011356b175e0cb8a01378ed on mdw771:master into on tomography:dev.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling f8baf6db5e95ade3a011356b175e0cb8a01378ed on mdw771:master into on tomography:dev.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 9197326722e4f9033f48b315869d8a148b41dd0e on mdw771:master into on tomography:dev.

carterbox commented 7 years ago

Travis failed because test_probe_circular. This test fails like 1/10 times because #53, so I'm ignoring it.