https://nf-co.re logo
Join Slack
Powered by
umitools shouldn't be hardcoding random seeds: <gi...
# request-review
j
umitools shouldn't be hardcoding random seeds: github.com/nf-core/modules/pull/12662
s
Hard disagree
j
Why?
It's only enforcing artificial reproducibility.
Makes sense for testing scenarios (which is why I moved it to the test profile)
If a tool doesn't set a random seed by default, why should we? The variant you force by setting the seed isn't any more valid than a variant you'd get by setting another one.
If the user/ pipeline author wants to set a seed, they can
r
It would be nice if the seed that the tool used could be reported, so one could exctly reproducce if wanted
👍 1
s
why it is artificial?
it is enforcing reproducibility
in what way is it artificial?
I added these several years ago, because I found that it was changing the results in "production" runs, i.e. my downstream analysis was not quite deterministic without it
j
If something has a random seed, it has legitimate variability across runs, which we should understand. By setting a seed you're forcing it to be consistent, but not in any meaningful way. For example, say I have some technical replicates from a sequencer. I probably want to understand their variability in the context of the algorithms' I'm using's inherent variability. Our forcibly suppressing that variability is entirely unhelpful, and meaningless.
s
disagree
I think we are striving for reproducible science
j
Yes, results will change in production, because the algorithms are based on simulations
s
this is not a simulation
it is choosing between two reads at random
#C0749PAEU9F
j
Grrr
You can still set your seed, I just don't see why you have to force me to
Boils down to: 1. If your results change, meaningfully, because of the way someone sets a seed, then your results are nonsense. 2. If you want to reproduce results exactly in the name of science, fair play, write down the seed so you can supply it explicitly next time. 3. It's not for us to prevent folks from running tools 'sans seed', if that's what they choose to do
p
I think the solution suggested by @Rike Hanssen is the most transparent and rigorous. Let's have a default seed and let the users change it. The value is reported in the logs.
j
That's what we already have. Forcing a static random seed where the tool doesn't use one is exactly my problem.
👍 1
To put in another way (then I promise I'll stop): Say we do as I suggest and disable the random seed. Then say Student A runs the pipeline twice, and by some fluke of bioinformatics nonsense (it happens), their favourite gene comes out in differential analysis based on the results of run 1, but not run 2, because of differences in random behaviour. Isn't that useful information? Wouldn't it encourage healthy skepticism in Student A about solidity of their run 1 results? Do we really think they'd be better left in the misapprehension that there was something special about the particular random pick used in run 1?
p
I think that we cannot prevent misuse. Tools must be agnostic and open, that's why I support the idea of a default (for those who don't care, even random is fine) and the option to set it (for those who want full control and reproduce the results they got in the previous run ,even if it was random, hence the log).
j
100% on board with setting it when an active choice has been made to do so. But the way things are actively prevents the random option. I cannot NOT set a seed.
100 rainbow 2
p
The only thing that I don't see is a log of the used seed. At the moment in the PR, there's no seed set by default. So if people run it twice they may get different results and complain about reproducibility. The price of the increased flexibility is that now the seed should be saved somewhere.
s
For your extremely niche case, then you can patch the module 🤷
I think you could set to "none" if you really want randomness
j
It's not a 'niche' use case, extreme or otherwise.
I don't understand why we can't just leave the option to 
ext.args
like we always do. We don't usually force users to patch around arbitrary choices made in modules, why this one?
m
Maintainers meeting?
I do think it's a good enough topic to reach a decision that will satisfy noone
🤣 1
shakefist 1
Because the coming maintainers meeting is in a while, and we cannot wait, here is my two cents on this topic: I do see @Simon Pearce’s point of wanting pure reproducibility and @Jonathan Manning’s point of not wanting to hardcode this seed. For me, because of the way we write our tests, we currently need pure reproducibility (even Jon recognized it). And I do think his solution for the proposed modules would work nicely. But it would change existing behavior at a pipeline level. So for me, we can only do so if we were to add something params at pipeline level, to control the use of hardcoded seed or not. so either 
--deterministic
, 
--fixed_seeds
, 
--random
or whatever.
And of course, that would be a nice RFC, and we should write a blogpost about it
s
Personally, this particular module fucked up the reproducibility of my results and I had to recalculate everything having fixed it
🤣 1
Which was a not insignificant effort, given that I'd processed ~1000 samples or so
😱 1
j
I'm very much of the opinion that you should retain the ability to set that seed Simon.
s
I'm very much of the opinion that should be the default
and this is like "oh we shouldn't set a default prefix because then it requires people to think about what they are doing"
j
The thing is, my solution allows you to continue working the way you want. Your solution prevents me from configuring things the way I want.
Also, again, I don't think we should be overriding tool writers' choices, as a matter of policy. umitools' authors elected not to force a seed, why should we over-rule that?
p
My opinion is in line with @Maxime Garcia and I push even further, all modules that use a seed should have a special param for controlling it. A default value, which is the same value used today, and a way to change it.
j
Again, that's similar to the situation today, and the thing I have a problem with. I think pipelines should have a 
--seed
option or similar that allows people to work deterministically when the situation needs it- building a results corpus, writing a paper etc. But outside of that the results should be changing between runs. The tools are making random, arbitrary choices, and by pretending they're not we're perpetuating a fallacy.
p
Yes, I see your point, but here we are introducing a breaking change because silently the default is not working as expected anymore. Like what happened to Simon. A possible solution is to make the param selection explicit, at least people are aware of the change. Making it required force people to make a choice. It's the only thing I see that can mitigate the issue.
s
The first line of nf-co.re/docs/specifications/overview is:
Copy code
The nf-core specifications define standards and best practices for developing robust, reproducible, and maintainable Nextflow components and pipelines.
literally the first line in our documentation
(although I'd be the first to admit that I ignore our documentation often enough if it doesn't suit 😉 )
m
I actually think we all agree in the general idea
2 Views
Open in Slack
PreviousNext
Powered by