Skip to content

[#144] Add: test coverage for reverse primitive #146

Open
Magnus-Mage wants to merge 4 commits intoDyalog:mainfrom
Magnus-Mage:tests/reverse-tests
Open

[#144] Add: test coverage for reverse primitive #146
Magnus-Mage wants to merge 4 commits intoDyalog:mainfrom
Magnus-Mage:tests/reverse-tests

Conversation

@Magnus-Mage
Copy link

@Magnus-Mage Magnus-Mage commented Feb 15, 2026

Related Issue(s)

Description

Following ULLU framework guidelines, added comprehensive test coverage for the reverse primitive:

  • General tests: Element count, datatype, and rank preservation
  • Comparison tolerance tests: CT/DCT behavior with tolerant equality
  • Random data tests: 1000-element arrays for integers and characters
  • Hash collision tests: Internal hash table handling (if utility available)
  • Independent tests: Single element, two-element, and large (10k) arrays
  • Deep nested tests: Deep nesting and mixed-rank structure tests
  • Namespace tests: Reverse on namespace arrays
  • Enhanced system variable coverage: Added ⎕DIV loop, expanded ⎕CT values (0, 1×, 10×, 0.1×)
  • Standardized descriptions: Added testDesc function for consistent test output

Total coverage: 6640 assertions across 32 system setting combinations

Note: Not using RunVariations pattern for this suite - using explicit loops for better clarity and maintainability.

PR Checklist (Remove options that are not relevant)

  • This PR adds tests for a new primitive
  • Code coverage has been checked
  • Documentation complete (Decision docs, code comments, etc.)

@Magnus-Mage
Copy link
Author

For Reverse First, i prefer to use a seperate namespace. However, testing both in same test suite will reduce code reuse. Also, i had some trouble using RunVariations so i am leaving them out for now

@Magnus-Mage Magnus-Mage marked this pull request as draft February 15, 2026 13:34
@sloorush
Copy link
Member

Hi @Magnus-Mage, Thanks for the PR. Let me know when you're satisfied I can review it.

The meaning of code coverage that we have in the project is checking the sources to see if all the source code written for the primitive is used and covered. So I have unchecked the checkbox in the first message on the PR. :)

@Magnus-Mage
Copy link
Author

Starting out, I had four unnecessary nested loops iterating over ⎕DIV, ⎕FR, ⎕CT/⎕DCT, and ⎕IO, which are all irrelevant for monadic reverse since it only reorders elements.

I was also using RunVariations incorrectly at first, simply because I didn't fully understand its syntax and argument passing for monadic primitives, which got resolved after working through the documentation more carefully. The model function was also my mistake as I had used itself inside it, which defeats the entire purpose of having an independent reference implementation, since if is broken in the interpreter the model would be broken too, making the comparison useless. The correct model uses ⍤1 to apply arithmetic index reversal {⍵[(n+1)-⍳n]} to each rank-1 cell, where n←(≢⍴⍵)⊃⍴⍵ gets the last axis length without using . The guard 0=≢⍴⍵:⍵ is kept not to replicate previous behaviour but because ⍤1 cannot operate on rank-0 scalars. (Do correct me if i am wrong here)

Nested arrays, pointer arrays, and bracket-axis tests were initially put in but following the advice I have removed them from the test for now.

@Magnus-Mage Magnus-Mage marked this pull request as ready for review February 17, 2026 15:28
Copy link
Member

@sloorush sloorush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, this was very nice for a first PR. I have some comments, mostly nit picks.

cmplx←{⍵,-⍵}(0J1×⍳100)+⌽⍳100
Hcmplx←{⍵,-⍵}(100000000000000J100000000000000×⍳20)

:For case :In 'bool' 'i1' 'i2' 'i3' 'dbl' 'Hdbl' 'fl' 'Hfl' 'cmplx' 'Hcmplx'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we are not looping the tests over ⎕IO?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⎕IO should be covered in RunVariations but i can loop for explicit tests.

Comment on lines +65 to +67
:For case :In 'bool' 'i1' 'i2' 'i3' 'dbl' 'Hdbl' 'fl' 'Hfl' 'cmplx' 'Hcmplx'
data←⍎case
desc←testDesc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All, the data we have here is of the same datatype in the array. Is there a reason cross datatype tests were skipped?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-datatype numeric tests (e.g., i1,dbl) were skipped because APL type coercion automatically promotes mixed numeric arrays to a homogeneous array of the wider type before reverse operates on them. Per the APL Wiki on Arrays (https://aplwiki.com/wiki/Array)

"When a numeric array is formed from numbers with different types, all numbers are converted to a common type in order to be represented as a flat array."

For mixed numeric/character arrays: per Dyalog 20.0 documentation (https://docs.dyalog.com/20.0/programming-reference-guide/introduction/arrays/display-of-arrays/), these become "mixed arrays" which are displayed alongside "simple numeric" arrays but contain both types. I can add those specifically as provided.

If there's a specific cross-datatype scenario you think would expose different behavior in reverse that current tests miss, i can add accordingly.

@Magnus-Mage
Copy link
Author

I was busy with some stuff so i wasn't able to update for the additional tests as we discussed.

@sloorush Go ahead and let me know if this looks good.

@Magnus-Mage
Copy link
Author

The remaining tests should be for nested structures for numeric and mixed types. I can add them as soon as possible.

I have hit the wall after that, if you think there are any other cases that are still remaining, let me know. Furthermore, i still havent added other axes, better to open a seperate pr to add those or should be continued in this one only?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants