Skip to content

Added support for atleast_1d, atleast_2d, atleast_3d#689

Closed
hinriksnaer wants to merge 0 commit intoml-explore:mainfrom
hinriksnaer:main
Closed

Added support for atleast_1d, atleast_2d, atleast_3d#689
hinriksnaer wants to merge 0 commit intoml-explore:mainfrom
hinriksnaer:main

Conversation

@hinriksnaer
Copy link
Contributor

@hinriksnaer hinriksnaer commented Feb 14, 2024

Proposed changes

Feature addition for the following issue. The code is based on the numpy implementation. PyTorch provides similar functionality but is limited to only Tensors and implements it in the C++ backend. Perhaps this is a good place to start?

Any feedback regarding the documentation, tests and implementation would be much appreciated.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@@ -0,0 +1,15 @@
.. _utils:

At least Utils
Copy link
Member

Choose a reason for hiding this comment

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

Don't need a separate page in the docs for this, I would just put them in the ops.rst list.

return {k: tree_unflatten(v) for k, v in children.items()}


def atleast_1d(*values):
Copy link
Member

Choose a reason for hiding this comment

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

These should be implemented in C++ not Python and then bound to Python via Pybind (the same as all the rest of our ops).

Copy link
Member

@awni awni left a comment

Choose a reason for hiding this comment

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

These should follow the implementation of all of our other ops (C++ and then bound to Python). Are you up for doing the C++ implementation?

@hinriksnaer
Copy link
Contributor Author

Thanks for the feedback! Yes, I'll get started on implementing the functionality using C++. Just to be sure, I presume that you want the functionality to be added here, correct?

Please let me know if there is anything else you would want me to know before getting started.

@awni
Copy link
Member

awni commented Feb 15, 2024

That's exactly right!

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