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

Duplicated random and uniform options for perturb_baseline #106

Closed
rodrigobdz opened this issue Apr 5, 2022 · 6 comments · Fixed by #114
Closed

Duplicated random and uniform options for perturb_baseline #106

rodrigobdz opened this issue Apr 5, 2022 · 6 comments · Fixed by #114

Comments

@rodrigobdz
Copy link
Contributor

Subject: Random number generation

The random and uniform values for the perturb_baseline parameter in the metrics are semantically overlapping. While both return the same values, the random function is more restrictive regarding its bounds.

Giving the user the option between random and uniform can be confusing, it would be preferable to only offer uniform instead.

"random": float(random.random()),
"uniform": float(random.uniform(arr.min(), arr.max())),

image

@sebastian-lapuschkin
Copy link
Collaborator

I agree with this. can uniform be kept, and the bounds be made optionalparameters with default values 0 and 1? this would nicely collapse both cases.

further remark: why use the python built-in random number generator instead e.g. numpy? this is a question out of curiosity, not a suggestion, gut feeling says numpy might be faster when pulling lots of random values, which we usually will do in eg. perturbation tests.

further remarks on the module referenced by @rodrigobdz:
"black" is not necessarily arr.min(),
"white" is not necessarily arr.max().

think rgb-color images (where values only describe location x channe voxels. or time series data.
I advise to disambiguate and call it "max", "min", "zero", etc. max/min values might also be per-channel

an option to pass custom baseline values might be of interest (which somehow align with some axes of the input space via broadcasting)

@annahedstroem
Copy link
Member

Thank you both.

I agree that keeping the uniform option is better.

I've excluded the "random" option now, plus changed to np.random instead of random as well as added a deprecation warning incl. a description of how to achieve the same results with uniform as with random.

Please see PR/ commit here: 704e392

@rodrigobdz do you want to take a look?

@rodrigobdz
Copy link
Contributor Author

@annahedstroem I like that you added a deprecation notice for random 💯.

Here are a few occurrences still pending removal/modification in the same file:

import random

random.uniform(patch.min(), patch.max())

random occurrences scattered around in the project. Some of these are unused imports:

image

image

@annahedstroem
Copy link
Member

annahedstroem commented Apr 14, 2022

The fixes in utils.py were fixed in commits after. I've now also removed the import of random in all source code files.

I've left it in the tutorials as we might be using the random in other operations there.

@annahedstroem
Copy link
Member

Thank you @rodrigobdz

If there is no further comment, I'll close the issue.

@rodrigobdz
Copy link
Contributor Author

Thanks, we can close the issue then.

annahedstroem added a commit that referenced this issue Apr 16, 2022
…normalisation-ordering

Issues #55 #104, #106, #110 and #113: smaller bug fixes
aaarrti pushed a commit that referenced this issue Apr 9, 2023
…normalisation-ordering

Issues #55 #104, #106, #110 and #113: smaller bug fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants