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

Fix issue 639 #640

Merged
merged 3 commits into from
Mar 22, 2024
Merged

Fix issue 639 #640

merged 3 commits into from
Mar 22, 2024

Conversation

AngelEzquerra
Copy link
Contributor

To fix this issue I made 2 sets of changes:

  1. Add a check for rank == 0 to the DynamicStackArray's product function (which fixes the actual issue).
  2. Make it (a bit) harder to accidentally create a rank-0 tensor (by making newTensor and newTensorUninit create an empty rank-1 tensor by default).

It is still possible (and sometimes useful) to create a rank-0 tensor, but users will need to do so explicitly.

Similar to same change done to `newTensor` on the previous commit.
Up until now calling newTensor without arguments would create a rank-0 tensor which does not work well (e.g. it reports its size as size 0)! Instead we now create a rank-1 empty tensor when no shape is provided.

It is still possible to explicitly create a rank-0 tensor by explicitly passing an empty shape (i.e. `[]`) to newTensor (e.g. `newTensor[float]([])`). This can be useful to create "sentinel" values for procedures that take tensors as arguments.
This fixes mratsim#639.

While this adds an extra check to `size` which might be called frequently, I have not seen a major difference on several of the benchmarks.
@Vindaar Vindaar merged commit 9cf5b41 into mratsim:master Mar 22, 2024
6 checks passed
@Vindaar
Copy link
Collaborator

Vindaar commented Mar 22, 2024

Great! 🥳

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