Some basic cleaning on various Distributions classes #190
Conversation
|
|
||
| aNumber2 > 0 | ||
| ifFalse: [ self error: 'Illegal distribution parameters']. | ||
| ifFalse: [ self error: 'Illegal distribution parameters' ]. |
There was a problem hiding this comment.
Here, for me, it would be clearer if we could invert the clause and say
number < 0 ifTrue: [...]
There was a problem hiding this comment.
It would be aNumber2 <=0 ifTrue: [...] I guess.
hemalvarambhia
left a comment
There was a problem hiding this comment.
The build is red here. Do we need to add some missing programmer tests?
| ifNil: [ [ v1 := Number random * 2 - 1. | ||
| v2 := Number random * 2 - 1. | ||
| w := v1 squared + v2 squared. | ||
| w > 1 ] whileTrue: [ ]. |
There was a problem hiding this comment.
Could you help me understand why the block is empty here?
There was a problem hiding this comment.
I dunno, I can remove it.
| ]. | ||
| ^v1 | ||
| NextRandom | ||
| ifNil: [ [ v1 := Number random * 2 - 1. |
There was a problem hiding this comment.
This part is a little hard for me to follow to be honest. If you could factor out this block to an intention revealing method to begin which, it would be very helpful.
There was a problem hiding this comment.
I have no idea what the block is doing ...
There was a problem hiding this comment.
What? It's well known Box-Muller algorithm for transforming a uniform distribution into a normal distribution.
See https://en.wikipedia.org/wiki/Box%E2%80%93Muller_transform
We need a pair (v1,v2) inside the unit circle.
If it's not, we have to redraw another pair.
Just replace whileTrue: [ ]. with whileTrue.
There was a problem hiding this comment.
I replace whileTrue:[] with whileTrue. I add a comment for the moment to say that we are using the Box-Muller transform. Do you see a better way to be more intention revealing @hemalvarambhia ?
There was a problem hiding this comment.
You could extract act boxMullerAlgorithm method
|
The build is red, because the coverage decreased I guess. |
|
I do the extraction as boxMullerTransform. |
| ^v1 | ||
|
|
||
| | v1 | | ||
| NextRandom |
There was a problem hiding this comment.
I wondered what kind of variable this is. It seems global. Is that right?
There was a problem hiding this comment.
no this is a class variable.
| } | ||
|
|
||
| { #category : #information } | ||
| PMNormalDistribution class >> boxMullerTransform [ |
There was a problem hiding this comment.
What's interesting is that this is a class method, rather than an instance one. I presume this choice was made because NextRandom is a class variable?
There was a problem hiding this comment.
no because PMNormalDistribution class >> random is a class method.
No description provided.