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

Feature/map to vector #513

Closed
wants to merge 4 commits into from
Closed

Conversation

him666
Copy link

@him666 him666 commented May 7, 2019

added map_to_vector wrapper methods in order to solve the next issue
#512

tried to avoid breaking changes since the Gem relies a lot on the normal map method that returns an array.

context "#map_to_vector!" do
it "maps to vector destructive" do
@common_all_dtypes.map_to_vector! { |v| v }
expect(@common_all_dtypes).to eq(Daru::Vector.new([5, 5, 5, 5, 5, 6, 6, 7, 8, 9, 10, 1, 2, 3, 4, 11, -99, -99]))
Copy link
Member

Choose a reason for hiding this comment

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

The line length should not go > 79 characters.


it "maps to vector destructive" do
@common_all_dtypes.map_to_vector! { |v| v + 1}
expect(@common_all_dtypes).to eq(Daru::Vector.new([6, 6, 6, 6, 6, 7, 7, 8, 9, 10, 11, 2, 3, 4, 5, 12, -98, -98]))
Copy link
Member

Choose a reason for hiding this comment

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

The line length should not go > 79 characters.

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

Other than the comment it is good to go.

Thanks for the PR

@kojix2
Copy link
Member

kojix2 commented May 12, 2019

Thank you for your PR.

But to be honest, I don't want to add the map_to_vector method.
Because recode method already exists.

The problem is that the recode method is hard to find.
The naming of map_to_vector may be better than recode.

However, my opinion is that the behavior of map should be that of recode or map_to_vector.
And the to_a method is sufficient to convert a Daru::Vector to an Array.

But I'm not sure if this idea is really good.
I will read Daru's code this month..

@Shekharrajak
Copy link
Member

@kojix2 , yes! I was thinking the same. Also it is written in recode docs. @him666 , please have a look whether map is overridden or not

@v0dro v0dro closed this May 30, 2020
@v0dro
Copy link
Member

v0dro commented May 30, 2020

Closing since we need a changed map method.

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.

4 participants