Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

Add deterministic option #465

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

xukai92
Copy link
Contributor

@xukai92 xukai92 commented Oct 26, 2019

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
Copy link
Contributor Author

xukai92 commented Nov 8, 2019

@maleadt Any comments on this?

@MikeInnes
Copy link
Collaborator

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?

@xukai92
Copy link
Contributor Author

xukai92 commented Jan 7, 2020

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

src/dnn/CUDNN.jl Outdated Show resolved Hide resolved
@MikeInnes
Copy link
Collaborator

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

Co-Authored-By: Mike J Innes <[email protected]>
@maleadt
Copy link
Member

maleadt commented Jan 21, 2020

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.

@xukai92
Copy link
Contributor Author

xukai92 commented Feb 11, 2020

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants