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

Index#dup should copy reference to name too #477

Merged
merged 2 commits into from
Jan 29, 2019

Conversation

Yuki-Inoue
Copy link
Contributor

Currently, if idx.instance_of?(Daru::Index), then following statement does not hold:

idx.dup.name == idx.name
# => returns false!!

This is because name is not copied on Daru::Index#dup, which I think is not the expected behavior at least to the library user.

So, this PR fixes it.

@Yuki-Inoue
Copy link
Contributor Author

ping @v0dro

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.

Thanks for noting this point.

@Shekharrajak
Copy link
Member

Shekharrajak commented Jan 22, 2019

I think == should check name of the index as well :

[28] pry(main)> dii = di.dup
=> #<Daru::Index(4): {speaker, mic, guitar, amp}>
[29] pry(main)> dii == di
=> true

This should return false, WDYT?

update

I think we should have #equal method which will compare only the values it contains with same data type (in this case - index array element). Similar method should be present in Vector , DataFrame, MultiIndex to just compare the elements (ignoring column name index name/value).
What do you think ?

@Shekharrajak
Copy link
Member

This PR looks good to fix the issue mentioned above. But it came up another question : Mentioned in above comment.

@Shekharrajak
Copy link
Member

I will wait for 24 hours for any other comments. If fine then I will go ahead and merge.

@Yuki-Inoue
Copy link
Contributor Author

Yuki-Inoue commented Jan 29, 2019

@Shekharrajak Hi, thanks for the feedback.

The problem fixed by this PR and the concern brought by your comment is, IMHO, a separate concern, so I'd be glad if you go ahead and merge this.

The concern you brought up, although I, myself, haven't encountered such a case where I need to compare the Daru objects based on the content of its data, I can agree that such method will be useful.

About the naming of such method, I disagree with naming the method as #equal. The reason is that in Ruby, following methods are used for equality comparison, and implementing a new method named #equal is somewhat confusing.

equals?(other)   <-> python `is` correspondent
eql?(other)         -> used for objects with `hash` method, for hash equality check
==(other)            <-> python `==` correspondent
===(other)         -> used for `case` matching

If such a method is needed, I'd recommend using a name which does not conflict with the ruby's equality methods. For example, how about data_eq?(other) or content_eq?(other)?

@Shekharrajak
Copy link
Member

@Yuki-Inoue , yes we need to discuss about my concern in different issue ticket (Ruby doc about ==, eq ,equal ) and it is different from this PR.

.dup should duplicate the all class attributes. So it should duplicate the name as well. It looks good Vector , DataFrame and MultiIndex already.

This PR is in.

@Shekharrajak Shekharrajak merged commit 445926b into SciRuby:master Jan 29, 2019
@Yuki-Inoue Yuki-Inoue deleted the dup-name branch March 10, 2019 12:13
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.

None yet

2 participants