Skip to content

Fixes and minors #129

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

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Fixes and minors #129

merged 2 commits into from
Apr 27, 2023

Conversation

lezcano
Copy link
Collaborator

@lezcano lezcano commented Apr 26, 2023

Minor improvements here and there e.g.

  • prefer flatten over ravel as it's more PyTorch-y
  • Prefer .double() or .long() over to(float) (I didn't even know that worked...) for the same reason
  • Don't use out= explicitly,
  • Improved a bit the implementation of average.
  • Don't call .item() if we can avoid it (added to the list of differences)
  • remove _mappings
  • remove the need for semi_private methods
  • Fixed ndarray.fill, as the normalizer was not working because of the future annotations

@lezcano
Copy link
Collaborator Author

lezcano commented Apr 26, 2023

I'll move the reductions out of _funcs_impl.py in a different PR, as this one was getting a bit too large already.

Don't use out= explicitly, improved a bit the implementation of
`average`.

Minor improvements here and there e.g.
- prefer flatten over ravel as it's more PyTorch-y
- Prefer `.double()` or `.long()` over `to(float)` (I didn't even know
  that worked...) for the same reason
- Don't call .item() if we can avoid it (added to the list of
  differences)
- remove _mappings
- remove the need of semi_private methods
- Fixed ndarray.fill, as the normalizer was not working because of the
  future annotations
"""Ravel the arrays if axis is none."""
# XXX: is only used at `concatenate` and cumsum/cumprod. Inline unless reused more widely
def axis_none_flatten(*tensors, axis=None):
"""Flatten the arrays if axis is None else normalize the axis."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit does not normalize the axis.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I was trying something else here but I didn't like it in the end. Forgot to update the comment

@ev-br
Copy link
Collaborator

ev-br commented Apr 27, 2023

Don't call .item() if we can avoid it (added to the list of differences)

I don't see it in #73, is this where you meant to add it ?

@lezcano
Copy link
Collaborator Author

lezcano commented Apr 27, 2023

Yes, I added it, but after some editing I guess it was deleted along with the line about lists and tuples. This is fixed now.

@lezcano lezcano merged commit 9c627db into main Apr 27, 2023
@lezcano lezcano deleted the review branch April 27, 2023 08:38
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