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 isapprox support for dataframes. #2373

Merged
merged 11 commits into from
Aug 23, 2020
Merged

Conversation

Sov-trotter
Copy link
Contributor

@Sov-trotter Sov-trotter commented Aug 20, 2020

Based on #2169
@bkamins For now I went on with the way Base.isapprox handles numeric values. And used broadcasting for column elements. Is this the right way to go?

@Sov-trotter Sov-trotter marked this pull request as draft August 20, 2020 18:42
@Sov-trotter
Copy link
Contributor Author

Sov-trotter commented Aug 20, 2020

In this latest commit I am using the isapprox method for individual columns defined https://github.com/JuliaLang/julia/blob/e58bc723ed7b1c10da28f55385c243f6df5499cf/stdlib/LinearAlgebra/src/generic.jl#L1631-L1642 here. One thing is that norm::Function=abs throws error since it's nowhere broadcasted in the isapprox for AbstractVector, in fact it's using the norm method by default, while in the commented out code(manual implementation), we were able to broadcast the abs function over to the elements.

@bkamins bkamins added feature non-breaking The proposed change is not breaking labels Aug 21, 2020
@bkamins bkamins added this to the 1.0 milestone Aug 21, 2020
@bkamins
Copy link
Member

bkamins commented Aug 21, 2020

Also can you please update NEWS.md with an entry about this addition (it should be mentioned as new functionality). Thank you!

@Sov-trotter
Copy link
Contributor Author

Yeah. Will do it. Thanks for helping out. :)

@Sov-trotter
Copy link
Contributor Author

Should I add LinearAlgebra into the dependency changes in News.md?

@bkamins
Copy link
Member

bkamins commented Aug 21, 2020

No, because it is a part of a standard library and in particular for LinearAlgebra you do not specify version number in [compat].

The idea of "Dependency changes" is to inform users of possible dependency conflicts, but in this case there is no such risk (everyone has LinearAlgebra installed anyway).

@Sov-trotter Sov-trotter marked this pull request as ready for review August 21, 2020 08:26
src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved
src/DataFrames.jl Outdated Show resolved Hide resolved
Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

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

Sorry - I have forgotten that the docstring is missing. Can you please add it before merging?

@Sov-trotter
Copy link
Contributor Author

Oh. Really sorry, it skipped my mind.

Sov-trotter and others added 2 commits August 22, 2020 00:56
Co-authored-by: Bogumił Kamiński <[email protected]>
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Looks good apart from two minor things.

src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved
src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Member

Ah, and it looks like you need to add LinearAlgebra to Project.toml to fix the CI failures. The line is:

LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

Co-authored-by: Milan Bouchet-Valat <[email protected]>
@bkamins
Copy link
Member

bkamins commented Aug 22, 2020

it looks like you need to add LinearAlgebra to Project.toml

It was added earlier - I wonder why it is now gone.

@Sov-trotter
Copy link
Contributor Author

It must have gone, when I rebased the branch onto master. Sorry for that.

@bkamins
Copy link
Member

bkamins commented Aug 23, 2020

Additionally functions.md in the documentation has to be updated. I think a new section Other is required for this function as it does not fit any section we have now.

@Sov-trotter
Copy link
Contributor Author

Won't it be "equality"? Since I see isequal is also missing in functions.md?

@bkamins
Copy link
Member

bkamins commented Aug 23, 2020

We do not have a docstring for isequal and == so that is why it is missing. Not sure if we should (they do what I guess people assume they do, in isapprox this is different as we compare columns)

@bkamins
Copy link
Member

bkamins commented Aug 23, 2020

But we can add Equality section I think

Add isapprox to equality section in functions


Add isapprox to equality section in functions
NEWS.md Show resolved Hide resolved
Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature non-breaking The proposed change is not breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants