Add Photocurrent Mapping Reconstruction#165
Conversation
| return torch.sign(v) * torch.clamp(torch.abs(v) - tau, min=0.0) | ||
|
|
||
|
|
||
| def estimate_lipschitz( |
There was a problem hiding this comment.
This already exists :)
from LION.utils.math import power_method
There was a problem hiding this comment.
Thank you for pointing that out! I can see that LION.utils.math.power_method is using numpy. Is it ok to edit it to use torch.Tensor instead? If it breaks some existing code maybe we can add a separate power_method_torch?
An alternative would be to convert from torch.Tensor to/from np.ndarray before/after the call, either inside or outside the function.
There was a problem hiding this comment.
Agree, maybe worth making the already existing one take either torch or numpy? flexible to both?
There was a problem hiding this comment.
Great idea! This might not be a problem right now but just in case there's some special kind of experiment where this function is called multiple times, I think it would be nice to also have a sub-function that just inputs tensors, computes on tensors, and returns tensors without ever leaving the GPU and making extra if checks. We can then wrap both numpy and torch versions together, perhaps have a power_method_np and power_method_torch, and then have a common power_method that does a check and choose the appropriate backend? This way we can be flexible: power_method for quick prototyping, and power_method_torch for training/GPU-only processing where performance is more critical? There may be a bit of code duplication in the two sub-functions but it might be worth the compromise, perhaps we might even find that one backend benefits more from a slightly different implementation than the other's, in which case it would be easier if they were already in separate places 😄
I'd also like to add unit tests for the power method where we have analytically computed values that we can check against, would appreciate any suggestions!
There was a problem hiding this comment.
Also a general direction question: for all operators, should we aim for supporting both np.ndarray and torch.Tensor like Tomosipo? Personally I think it's much simpler to focus on torch.Tensor (Tomosipo can remain a special case but we could encourage users to use Pytorch anyway) since, if I understand correctly, we mainly provide tools for training models. Adding numpy support for everything might make code more complicated and brittle since numpy and Pytorch don't always have the same API (e.g. numpy uses .ndim while Pytorch uses .dim())
There was a problem hiding this comment.
Sounds good! but in any case, a type check in an if condition should be computationally free, so that should not be an issue. But your solution sounds great. For the unit test, that is hard to do without an explicit matrix where we can compute the biggest eigenvalue. The method itself should converge, maybe that is what we can do as unit test, but I think this is more a thing to use in the opposite way: use power method as unit test for operators not changing.
for operators: agree. We can just support Torch for sure, and numpy optional. Specific operators can chose to support it or error otherwise (we can have an overridable method that errors by default, or something like that).
cf437dd to
5577cfb
Compare
c138322 to
5e284e0
Compare
5e284e0 to
11b946d
Compare
24041cb to
954c61e
Compare
954c61e to
fc05c0d
Compare
|
Can we change TomopgarphicProjOP to CTOP or something like that? tomography is quite wider than the Radon transform, and even within Radon there is much more geometries than CT, so this uniquely for CT. |
|
Also, lets put iphython notebooks in a separate folder, I prefer example scripts to be purely python for now |
|
great! should I merge? |
|
Yes, I believe it's ok for this PR now! I made some further small changes:
|
Add Photocurrent Mapping Reconstruction
Main changes:
Operatorclass to support new operators. This is mainly to create a common interface for all algorithms which expect aforwardoperation and aadjointoperation.make_operatorto return aTomographicProjOpwhich is a wrapper around tomosipoOperator). Ensure backward compatibility and autograd functionality so that all current code usingmake_operatorstill works.PhotocurrentMapOpfor photocurrent mapping (PCM) and other operatorspower_methodwork with tensor instead of ndarray, assumeTomographicProjOpalso only works with tensorOther important changes:
pyproject.toml: Add dependencies for wavelet transform, ignore some import warnings so that tests can run, ...README.md(TODO: Renameenv_base.ymltoenv.ymland place the old one inmisc)Minor changes:
from __future__ import annotationsto enable safe usage of|in function signatures, reorder imports for consistency, switch fromnp.producttonp.prod, ...Notes:
scripts/example_scripts/LPD.pyran without issuesFuture improvements:
PyWaveletsandspgl1which usenumpybackend. ImplementPyTorchversions.pytestto Github action and set it to manual so that reviewers can choose to run them just before merging to save Github compute allowance. Only allow tests to run on CPU on Github.LION.operators(no need to check for all correctness, just test execute (cover) every line)