search

TomographicImaging / CIL

A versatile python framework for tomographic imaging
https://tomographicimaging.github.io/CIL/
Apache License 2.0
88 stars 40 forks source link

FGP_TV nonnegativity to be changed from True to False #1825

Closed jakobsj closed 2 weeks ago

jakobsj commented 3 weeks ago

Description

The FGP_TV implementation of total variation has its nonnegativity input set to True by default: https://tomographicimaging.github.io/CIL/nightly/plugins/#total-variation

This is not intuitive and causes unexpected signal clipping to nonnegative values when user simply want to employ TV regularization. While commonly run on standard CT data where nonnegativity is often desirable, this should be selection. In general, users may want to run on data with some/all negative values, for example after phase retrieval or in other than CT applications, and default should be changed to False.

Environment

import cil, sys
print(cil.version.version, cil.version.commit_hash, sys.version, sys.platform)

Not available (told by collaborator)

epapoutsellis commented 2 weeks ago

This is a major change. Besides checking unittest (FGP_TV vs CIL Totalvariation), all CIL-demos, CIL-papers and maybe CIL-Showcase should be checked because reconstructed images will probably be wrong.

MargaretDuff commented 2 weeks ago

This is a major change. Besides checking unit test (FGP_TV vs CIL Totalvariation), all CIL-demos, CIL-papers and maybe CIL-Showcase should be checked because reconstructed images will probably be wrong.

Yes, I was surprised to find the unit tests passed without changes, but it looks like the nonnegativity is set explicitly when FGP_TV is compared to CIL TotalVariation.

As to CIL-demos, CIL-papers and CIL-showcase. For the CIL-showcase we explicitly say which version of CIL we use. For the papers, do you know if we also mention a version or a tag? The demos still suggest 22.2.0 and updating to a newer version is on our to-do list anyway.

I think I agree by default we shouldn't have a non-negativity constraint. It would also then match CIL Totalvariation, which has no upper or lower values by default.