Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOC: Improve docstring of IntensityTransform #31

Open
balbasty opened this issue Jan 27, 2025 · 0 comments
Open

DOC: Improve docstring of IntensityTransform #31

balbasty opened this issue Jan 27, 2025 · 0 comments
Labels
documentation Improvements or additions to documentation

Comments

@balbasty
Copy link
Owner

The IntensityTransform class (like many stochastic layers) has quite counter-intuitive arguments.

The issue is that each argument can receive either a Sampler (which is intuitive -- we'll then use that sampler to sample parameter values) or a scalar value. The counter-intutive bit is that in that case, we use this value as a bound or standard deviation of a Sampler that we instantiate ourselves. The documentation does state this, but after a bit of text so it's a bit hidden.

The other difficulty, is that the help text for each argument describes the effect of the sampled value -- not the effect of the Sampler's parameter. For example, the gamma docstring states:

gamma : float or Sampler or False
    The Gamma transform squeezes intensities such that the contrast
    to noise ratio is decreased (positive values lead to less
    decreased contrast, positive values lead to increased contrast).
    If a `float`, sample the gamma exponent from `LogNormal(0, value)`.

However, if a float is passed, it is used to instantiate the sampler LogNormal(0, gamma), so the effect of the gamma argument is not described in the docstring. (There's another issue in that particular docstring, in that we talk about positive/negative values of gamma, whereas we mean positive/negative values of log(gamma) -- i.e., values of gamma that are above or below 1.

I need to think of a better way of dealing with this but:

  • I don't want to name the argument gamma_std or bias_bound, because we can also pass an instantiated Sampler, so this makes no sense.
  • I could force users to always pass a Sampler, rather than the value of the parameter of a sampler, but it makes things a bit too verbose when all we want is changing a default bound.

What could help is to pass samplers as default argument, instead of values that gets converted to sampler, so when people look at the function, they see that by default a Sampler is passed -- not a value. And also order the possible types as "Sampler or foat or False", so that it is clear that the normal use is to pass a Sampler (which can be a Fixed sampler!), and that passing a float is a less standard use.

@balbasty balbasty added the documentation Improvements or additions to documentation label Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

1 participant