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

Add Vector.take and Vector.drop functions #3629

Merged
merged 27 commits into from
Aug 10, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Aug 1, 2022

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/182307048

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@radeusgd radeusgd self-assigned this Aug 1, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/index-sub-range-182306743 branch from e0685bd to f53c9a4 Compare August 1, 2022 10:55
@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from c37bc3d to bf2ca66 Compare August 1, 2022 10:58
@radeusgd radeusgd force-pushed the wip/radeusgd/index-sub-range-182306743 branch from 03e8768 to b46ae97 Compare August 1, 2022 15:29
@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from bf2ca66 to 8474e25 Compare August 2, 2022 09:55
@radeusgd radeusgd marked this pull request as ready for review August 2, 2022 09:59
@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from 8474e25 to 10e1608 Compare August 2, 2022 15:16
@radeusgd radeusgd force-pushed the wip/radeusgd/index-sub-range-182306743 branch from c435c83 to a496710 Compare August 2, 2022 15:17
@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from 10e1608 to e7764df Compare August 2, 2022 15:21
slice_ranges vector ranges =
if ranges.length == 0 then empty else case ranges.length == 1 of
True -> case ranges.first of
Integer -> Vector (Array.new_1 (vector.unsafe_at ranges.first))
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this special case, because it can be triggered relatively often by vec.take (By_Index 123). It seemed that creating a slice just to hold a single element will be an overkill - because the cost of allocating the atom representing the slice descriptor should be roughly the same as the cost of allocating a single-element array and the memory footprint of both of these is also comparable, so it seems to me that making a slice for just a single element would be wasteful.

Copy link
Member

Choose a reason for hiding this comment

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

We could do a faster short circuit earlier that if By_Index is an Integer or a single Integer Vector we return just self.at index and let the self.at do the checking and resolving,

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but then again I still need to do the check at slice_ranges for a single range - so I still need this case-of but now it will be in two places.

Base automatically changed from wip/radeusgd/index-sub-range-182306743 to develop August 3, 2022 11:41
@jdunkerley jdunkerley force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from c39dc9f to b6b17e5 Compare August 3, 2022 12:44
test/Benchmarks/src/Vector.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Vector_Spec.enso Outdated Show resolved Hide resolved
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Want to have a think on the normalize_ranges and invert but generally looks good.

slice_ranges vector ranges =
if ranges.length == 0 then empty else case ranges.length == 1 of
True -> case ranges.first of
Integer -> Vector (Array.new_1 (vector.unsafe_at ranges.first))
Copy link
Member

Choose a reason for hiding this comment

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

We could do a faster short circuit earlier that if By_Index is an Integer or a single Integer Vector we return just self.at index and let the self.at do the checking and resolving,

@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from 5513e6b to 46695cf Compare August 4, 2022 12:36
@radeusgd radeusgd force-pushed the wip/radeusgd/vector-take-drop-182307048 branch from 46695cf to 16a458f Compare August 10, 2022 14:09
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Aug 10, 2022
@mergify mergify bot merged commit 3dca738 into develop Aug 10, 2022
@mergify mergify bot deleted the wip/radeusgd/vector-take-drop-182307048 branch August 10, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants