Skip to content

Conversation

@jacobpake
Copy link
Collaborator

@jacobpake jacobpake commented Nov 19, 2025

This PR makes matrices 0-indexed (for consistency with lists), updating the internal and library functions.

[| (i, j) for (i, j) in (n, m) |]

The above produces a n x m matrix as before, but i and j here will be 0..n-1 and 0..m-1 rather than 1..n and 1..m (is this what we want?).

dims will return the size as before.

Internals diff
Library diff

Most tests were straightforward to update but there are two notable cases that could use another set of eyes:

array.fld - the values in the matrix are initialised from the indices, so the matrix (and test output) now contains different values (is this right?)

compute-dtw.fld - output is different, either similar to above or I may have made a mistake in test/fluid/lib/dtw.fld (this was a bit delicate)

Fixes #790

@rolyp
Copy link
Collaborator

rolyp commented Nov 20, 2025

Thanks @jacobpake -- array.fld looks as expected. compute-dtw.fld I'm less sure about -- I'll take a look this morning.

@jacobpake
Copy link
Collaborator Author

jacobpake commented Nov 20, 2025

compute-dtw.fld I'm less sure about -- I'll take a look this morning.

Thanks - I struggled to understand the calculateDTW functions - perhaps it could be clearer when / why we offset indices and add other documentation.

rolyp
rolyp previously approved these changes Nov 20, 2025
@jacobpake jacobpake marked this pull request as ready for review November 20, 2025 16:51
@jacobpake
Copy link
Collaborator Author

@rolyp I made the changes to range we discussed here.

I change ListEnum to use a new range_inclusive. Let me know if it should keep using range (and have the + 1 in exprFwd/exprBwd) instead or if these changes should happen in a separate PR.

Copy link
Collaborator

@rolyp rolyp left a comment

Choose a reason for hiding this comment

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

Looks great, just some minor suggestions re. the new exprBwd case for [e1 .. e2].

@rolyp rolyp merged commit 665139c into develop Nov 21, 2025
6 checks passed
@rolyp rolyp deleted the 790-matrix-0-index branch November 21, 2025 10:46
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.

0-based array indexing

3 participants