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 diagm for vectors #31125

Merged
merged 8 commits into from Mar 1, 2019

Conversation

@eulerkochy
Copy link
Contributor

commented Feb 20, 2019

This in reference to #31111 . I've added the missing function definition.

@StefanKarpinski
Copy link
Member

left a comment

Thanks for the contribution! This should also add a test for the method, checking that it works as expected as well as documentation and a NEWS entry.

stdlib/LinearAlgebra/src/dense.jl Outdated Show resolved Hide resolved
stdlib/LinearAlgebra/src/dense.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

I've removed the un-necessary commit from the branch!

base/dict.jl Outdated Show resolved Hide resolved
@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

At this point I would just do git fetch to pull the latest master and then git reset origin/master and edit the code until the diff looks the way you want it to. This is a small change and the git gymnastics required to do this via commits and rebasing aren't really worth it.

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from 07f9d59 to 8cf8d17 Feb 20, 2019

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Is it fine now?

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from 88ec94a to e7424bf Feb 21, 2019

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Is it fine now?

If you look at https://github.com/JuliaLang/julia/pull/31125/files you can see that many irrelevant files seems to have been deleted in this PR. There is also still no test added.

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Tests as in documented examples of the function call? If so, what's the format to be followed?

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Tests as in testing the functionality of the new feature (using the @test macro), see e.g. https://github.com/JuliaLang/julia/blob/master/CONTRIBUTING.md#writing-tests.

Also we could update all the instances of syntax like diagm(0 => D.diag) in the code to use diagm(D.diag) instead.

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Interesting! But can I make a separate PR for that? This PR has too clumsy for a lapse on my part and I'm not proud of this fact. :(

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Sure, that can very well be in another PR.

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from f79d779 to 7a5e5b4 Feb 21, 2019

@KristofferC KristofferC force-pushed the eulerkochy:add-diagm branch from 7a5e5b4 to 1d1722a Feb 21, 2019

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Ok, what's left to do in this PR? Can anyone tell? @KristofferC @StefanKarpinski

@pkofod

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

Ok, what's left to do in this PR? Can anyone tell? @KristofferC @StefanKarpinski

No need to ping when people are already in the conversation :)

I did note the conversation above, but would it really be such a hassle to simply include the tests in this PR? It's easier to find at a later point if it's in the same PR, and you'll actually be 110% confident that your changes work as intended. Even the smallest changes can have unintended consequences, so it's just good practice IMO. I get the eagerness to have the PR merged, but honestly, there's not going to be a new release of Julia tomorrow (I don't think there is at least ! ) so if this is merged now or if it's merged three days from now WITH tests really doesn't matter from an end-users perspective - so I'd say the latter (wait and merge with tests) makes sense. Even for such a "simple" (though appreciated) PR.

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Thanks for the advice, and well I was just curious to know what remaining work was left so that I could complete it. Seems like to add tests for it!

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch 2 times, most recently from 6f5a7dd to 4e22ff4 Feb 27, 2019

@eulerkochy eulerkochy closed this Feb 27, 2019

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from 4e22ff4 to 9732693 Feb 27, 2019

@eulerkochy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Just trying to reopen!

@eulerkochy eulerkochy reopened this Feb 28, 2019

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from 997a4ba to 16eafc4 Feb 28, 2019

@eulerkochy eulerkochy force-pushed the eulerkochy:add-diagm branch from 16eafc4 to 16c5061 Feb 28, 2019

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Getting there!

StefanKarpinski and others added 2 commits Feb 28, 2019
Update stdlib/LinearAlgebra/src/dense.jl
Co-Authored-By: eulerkochy <kc99.kol@gmail.com>
NEWS.md Outdated Show resolved Hide resolved
StefanKarpinski and others added 2 commits Feb 28, 2019
Update NEWS.md
As per review.

Co-Authored-By: eulerkochy <kc99.kol@gmail.com>
@StefanKarpinski
Copy link
Member

left a comment

Looking good. Glad you stuck it out!

@fredrikekre fredrikekre merged commit 5a4191d into JuliaLang:master Mar 1, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.