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

NEWS entry for sprand changes #32554

Closed
StefanKarpinski opened this issue Jul 11, 2019 · 11 comments · Fixed by #32695
Closed

NEWS entry for sprand changes #32554

StefanKarpinski opened this issue Jul 11, 2019 · 11 comments · Fixed by #32695
Labels
docs This change adds or pertains to documentation help wanted Indicates that a maintainer wants help on an issue or pull request sparse Sparse arrays

Comments

@StefanKarpinski
Copy link
Member

This change #30494 needs NEWS.

@StefanKarpinski StefanKarpinski added docs This change adds or pertains to documentation help wanted Indicates that a maintainer wants help on an issue or pull request labels Jul 11, 2019
@StefanKarpinski StefanKarpinski added this to the 1.2 milestone Jul 11, 2019
@abraunst
Copy link
Contributor

something like this?

  • The sprand function is now 2 to 5 times faster ([Faster and simpler sprand for SparseMatrixCSC #30494]). As a consequence of this change, the random stream of matrices produced with sprand and sprandn has changed. If you need to reproduce matrices prior to this change, your best bet is to use the old version of the sprand function that you can find here .

@ViralBShah
Copy link
Member

I think this is fine. I think if someone wants to reproduce the old stream, they should use the older released version of Julia. My view is that we don't want to put it in the NEWS.

@ViralBShah ViralBShah added the sparse Sparse arrays label Jul 11, 2019
@tpapp
Copy link
Contributor

tpapp commented Jul 12, 2019

I think that in general, it could be emphasized somewhere in the docs that unless explicitly stated otherwise, random APIs in Base and the standard libraries can break reproducibility of random streams/bits without deprecation or prior notice.

@abraunst
Copy link
Contributor

I think this is fine. I think if someone wants to reproduce the old stream, they should use the older released version of Julia. My view is that we don't want to put it in the NEWS.

Ermm.. I don't understand if you are ok with the proposed addition to NEWS or not. 😕

@StefanKarpinski
Copy link
Member Author

My view is that we don't want to put it in the NEWS.

What? Why would we not put changes to random number sequences in NEWS? That is definitely a "minor change".

@ViralBShah
Copy link
Member

Sorry for the ambiguity. My proposal was to put the following in the NEWS:

The sprand function is now 2 to 5 times faster ([#30494]). As a consequence of this change, the random stream of matrices produced with sprand and sprandn has changed. If you need to reproduce matrices prior to this change, please use an older release of Julia.

@abraunst
Copy link
Contributor

Sorry for the ambiguity. My proposal was to put the following in the NEWS:

The sprand function is now 2 to 5 times faster ([#30494]). As a consequence of this change, the random stream of matrices produced with sprand and sprandn has changed. If you need to reproduce matrices prior to this change, please use an older release of Julia.

It's fine for me either way, but I think it is pretty nice that you can essentially just cut&paste the old code and have a working oldsprand in julia 1.2 in no time. The fact that the libraries are written in Julia makes this possible and is a real strength WRT most other languages. I don't see why not pointing the user to the right commit to save him the search (search which is trivial to do if you have the sources installed, less trivial if you installed from binaries BTW). In my opinion, it would be nice if this could be part of julia's RNG policy and be done consistently...

@ViralBShah
Copy link
Member

Why not mention it in the documentation of the function? That's the place I would look into if I wanted the old behaviour - not in the NEWS. I guess it could be in the NEWS too, but may stop working with 2.0.

@KristofferC
Copy link
Member

Imo just point out the change. There is no need to say that one can get the old behavior by using the old code for every change, that is always true.

@jarlebring
Copy link
Contributor

jarlebring commented Jul 13, 2019

Another option is to put the link to the blob with the code for the old stream in the description of PR 30494, and not mention how to get the old stream in the news. The interested user would anyway first go to the PR, where you can also find further justifications in the discussion.

The sprand function is now 2 to 5 times faster ([#30494]). As a consequence of this change, the random stream of matrices produced with sprand and sprandn has changed.

@abraunst
Copy link
Contributor

Another option is to put the link to the blob with the code for the old stream in the description of PR 30494, and not mention how to get the old stream in the news. The interested user would anyway first go to the PR, where you can also find further justifications in the discussion.

Good idea, I did just that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation help wanted Indicates that a maintainer wants help on an issue or pull request sparse Sparse arrays
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants