CuArrays.jl icon indicating copy to clipboard operation
CuArrays.jl copied to clipboard

Add deterministic option

Open xukai92 opened this issue 4 years ago • 6 comments

Some of the commonly used kernels (conv and maxpool) are not deterministic by default. This hurts reproducibility a lot - when fix random seed (e.g. by seed!(1)) users cannot reproduce the results.

I added an option to provide default and check if a mode/algo is deterministic.

xukai92 avatar Oct 26 '19 23:10 xukai92

@maleadt Any comments on this?

xukai92 avatar Nov 08 '19 23:11 xukai92

This seems like a good idea, but I think we could do a slightly different setup:

  • Have a global var for determinism, and provide getters and setters for this (like isdeterministic now) rather than requiring eval
  • Document these and warn that some algo choices might be ignore if it is true
  • If isdeterministic, force the mode to be a deterministic one regardless of what was passed as a kwarg argument, rather than throwing an error dependent on mode.

How does that sound?

MikeInnes avatar Dec 09 '19 14:12 MikeInnes

@MikeInnes I introduced the global variable for determinism and replaced assertions with warnings when not deterministic.

xukai92 avatar Jan 07 '20 21:01 xukai92

Looks fine to me, thanks. Would like a thumbs up from @maleadt.

MikeInnes avatar Jan 10 '20 14:01 MikeInnes

I'm not against the feature, but I worry it will not be maintained and thus the determinism promise won't mean much. At the least needs some tests though, and it might be easier to test if the warning were just an error.

maleadt avatar Jan 21 '20 15:01 maleadt

At the least needs some tests though, and it might be easier to test if the warning were just an error.

Sure I will add some tests.

xukai92 avatar Feb 11 '20 21:02 xukai92