Proposed take: implementation

[hernanmd]

For PMMatrix and PMVector, which answer the binomial coefficient at each element for a given integer. These are two convenience methods which makes easier to write code using contingency tables (marginals and grand totals).

[hemalvarambhia]

I'd recommend being more specific with the test name here. You could do something like

testThatTakeYieldsACollection...

[olekscode]

I think that the name is good, but this test could be split in two:

testTake
	| vector |
	vector := #(1 2 3) asPMVector.
	self assert: (vector take: 2) equals: #(0 1 3) asPMVector.
	self assert: (vector take: 3) equals: #(0 0 1) asPMVector.

testTakeEmpty
	| vector |
	vector := #() asPMVector. 
	self assert: (vector take: 2) equals: #() asPMVector.

[hemalvarambhia]

I'd disagree that the name is good because, looking at the code snippet, I can't tell how vector take: 2 results in [0, 1, 3]

[hemalvarambhia]

Other tests you can add include:

• when the argument value is more than the dimension of the vector; and
• the argument of take is 0 or negative

What happens in those cases?

[hemalvarambhia]

Again, here, could you be more specific about the behaviour you're testing?

[hernanmd]

HI @hemalvarambhia the selector is the one created when you right-click on a method and select "Jump to test method". This way methods are connected with their tests and recognized with the green/red circle in the method name list. It is a very common way to create test methods in Pharo.

[olekscode]

Indeed, PMVectorTest >> testTake is consistent with PMVector >> take:.
And PMVector >> take: is consistent with Integer >> take:

However, I think that Integer >> take: should be given a more meaningful name (e.g. numberOfCombinationsTaken: or binomialCoefficientsWith:).

Also, the implementation of Integer >> take: is a bit unclear:

• comment is hard to read
• names kk, num, denom, factor could be better

Integer >> take: kk
	"Return the number of combinations of (self) elements taken kk at a time.  
	For 6 take 3, this is 6*5*4 / (1*2*3).  Zero outside of Pascal's triangle.  Use a trick to go faster."
	" 6 take: 3  "

	| num denom |
	kk < 0 ifTrue: [^ 0].
	kk > self ifTrue: [^ 0].
	num := 1.
	self to: (kk max: self-kk) + 1 by: -1 do: [:factor | num := num * factor].
	denom := 1.
	1 to: (kk min: self-kk) do: [:factor | denom := denom * factor].
	^ num // denom

But this is outside PolyMath
We should create an issue for that in pharo repository

UPD: here is the new issue: pharo-project/pharo#7967

[hemalvarambhia]

@hernanmd with the way you describe generating test methods in pharo, I read from that that this implies a 1:1 correspondence between message to it's test ie a message has only one test and that you must have a test method of the form test{methodName} . Have I understood you correctly? What happens when we write multiple tests per message? What does Pharo do then? Assuming I have understood you, whilst this correspondence may be good for code navigation, we lose the ability to understand the tests as, potentially, we end up with tests that are long, intricate and hard to understand. In Gerard Mezsaros' 'XUnit Test Patterns' work, he recommends writing tests for humans and values tests as documentation.

As a compromise would you be will to rename the test method names to something like

testTakeReturnsXYZWhenScenario?

[hemalvarambhia]

I had a quick look at contingency tables and marginal totals on the wiki, and it looks like these ideas are from Statistics. Should we introduce these in a Statistics module in PolyMath?

[hemalvarambhia]

@SergeStinckwich Could you enable the check that a PR needs approval from a maintainer before the merge button goes green? We seem to have lost this.

[SergeStinckwich]

@SergeStinckwich Could you enable the check that a PR needs approval from a maintainer before the merge button goes green? We seem to have lost this.

I think there was nothing like that. There was only a review check.
I modify the settings in order to require one code review before being able to merge the code.

[hernanmd]

@hemalvarambhia I read your question. Nothing prevents creating additional tests for a single method. And there is no limit in creating 1,2,3,N tests. What I described is a facility from the System Browser to jump and execute the method test directly from the Browser. Other test implementations of the #take: could have more descriptive method names, but it will not be linked with the method (because they use a different selector sub-name as part of the selector),

@SergeStinckwich let me know if you are ok with continue using the test{Selector} pattern auto-generated from Calypso menu which enables linking methods with its tests. Or use the suggestion from hemalvarambhia?

[hemalvarambhia]

@hernanmd I don't recall learning about the idea described in the comment in Linear Algebra (Pure Mathematics). This could be my ignorance so could you point me to a reference?

If this isn't a Pure Mathematics idea, and given it looks like one from Statistics (contingency tables etc.), should we categorize it as such? Open question