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

Assume row-major order for indices #10

Merged
merged 8 commits into from
May 20, 2021
Merged

Conversation

Mandrenkov
Copy link
Collaborator

Context:
The modern convention is to specify indices in row-major order; however, Jet expects indices to be in column-major order.

Description of the Change:

  • RavelIndex() and UnravelIndex() now assume indices are specified in row-major order.
    • A few sanity checks have also been added and an edge case (where shape has one entry) has been removed.
  • The ShapeToSize() function has been moved from TensorHelpers.hpp to Utilities.hpp.
  • The [utilities] tag has been replaced with [Utilities] for the sake of consistency.

Benefits:

  • Writing indices is slightly more intuitive.
  • Indices map directly to the NumPy index raveling/unravelling functions.

Possible Drawbacks:

  • Existing code that specifies a multi-index directly (in column-major order) will need to be adjusted.

Related GitHub Issues:
None.

@Mandrenkov Mandrenkov added the code quality 🎓 Improvements to code quality label May 19, 2021
@github-actions
Copy link

github-actions bot commented May 19, 2021

Test Report (C++) on Ubuntu

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
239 tests +4  239 ✔️ +4  0 💤 ±0  0 ❌ ±0 
519 runs  +2  519 ✔️ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 59236b8. ± Comparison against base commit 7635ed3.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 19, 2021

Test Report (C++) on MacOS

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
239 tests +4  239 ✔️ +4  0 💤 ±0  0 ❌ ±0 
519 runs  +2  519 ✔️ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 59236b8. ± Comparison against base commit 7635ed3.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@brownj85 brownj85 left a comment

Choose a reason for hiding this comment

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

Code quality 📈 📈

Copy link
Contributor

@trevor-vincent trevor-vincent left a comment

Choose a reason for hiding this comment

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

Thanks for doing tihis Mikhail, now we're super consistent.

@Mandrenkov Mandrenkov merged commit 6e7f624 into enable-exceptions May 20, 2021
@Mandrenkov Mandrenkov deleted the row-major-indices branch May 20, 2021 13:20
mlxd added a commit that referenced this pull request May 21, 2021
* Replace terminate with throw

* Add Jet exception and implement tests

* Remove code paths protected by static_assert

* Update testing to check exception output

* Update Abort docstrings

* Add tests for Abort.hpp

* Fix linting of tests

* Remove unneeded function

* Update JetException docstring

* Fix linting

* Remove Fatal word

* Rename invalid_tensor_file and extend from JetException

* Update CHANGELOG

* Assume row-major order for indices (#10)

* Switch multi-index to row-major order

* Replace 'utilities' tag with 'Utilities'

* Fix tests specifying indices in column-major order

* Update changelog

* Fix Python unit test using column-major indices

* Undo modification to previous changelog entry

* Fix PR number

* Update change-log

* Fix Abort formatting and tests

* Rename JetException and avoid naming collisions

* Fix class name in changelog

* Trigger CI

Co-authored-by: Mikhail Andrenkov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality 🎓 Improvements to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants