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

[RFC] Moved linalg tests into better named files #12420

Merged
merged 1 commit into from
Aug 2, 2015

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Aug 1, 2015

Many of the linalg tests were in files with opaque names like linalg1.jl. I've split up the tests so that all the QR tests are together in test/linalg/qr.jl. Some of the tests were being run over and over needlessly because of the complicated structure of linalg1.jl. Almost all the changes here are just copying code around to new files.

Some of the sparse linear algebra was tested in these dense linalg files. I've moved that out to where it belongs.

There are a lot of changes in this commit because it was really hard to try to split these mega-files into smaller pieces iteratively. I can go back and try to split this into multiple commits which will independently pass tests if people want.

Running on my 2012 MBA, the old version took 678 seconds to run, and the new version took 746 seconds.

If the decision is made to merge this, we should wait until after coverage is working again so we can make sure I haven't totally destroyed our linalg coverage.

cc: @andreasnoack @jiahao @tkelman @StefanKarpinski

@kshyatt kshyatt added test This change adds or pertains to unit tests linear algebra Linear algebra labels Aug 1, 2015
@ScottPJones
Copy link
Contributor

Did you try to make the test names match the corresponding names in base?
I tried to do that with my strings & unicode reorg, and I noticed that @StefanKarpinski also recommended doing something like that. Your reorg sounds nice!

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2015

Did you do the reorganization entirely by hand? For moving around a large amount of code like this it could help with verifying that nothing is being lost if it can be replicated via a script. Though that might be a fair amount more effort than needed. Depends how long this stays open and how many other test changes it needs to take into account in future rebases.

@kshyatt
Copy link
Contributor Author

kshyatt commented Aug 1, 2015

Yeah, it was by hand.

@ViralBShah
Copy link
Member

It seems like a lot of work to have to write a script. Is it not good enough if the coverage stays roughly the same? Something like this had to be done eventually.

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2015

I think it should be fine if coverage stays the same, and there's a bit of irony to @kshyatt saying "make sure I haven't totally destroyed our linalg coverage," but coverage runs on base are not running correctly right now due to JuliaLang/JuliaParser.jl#19.

@ScottPJones
Copy link
Contributor

@tkelman If indeed #11333 is causing this to fail, couldn't it just be removed for now? It seems that being able to run coverage while trying to get ready for a release candidate is more important than allowing some new syntax.

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2015

Coverage didn't start failing when the PR that enabled the syntax was first merged in May, coverage started failing about a week ago when that syntax first started getting used in base. So it would be a much better idea to just find a workaround alternate syntax at the location it's getting used, rather than revert parser changes.

@yuyichao
Copy link
Contributor

yuyichao commented Aug 1, 2015

@tkelman A few days ago @jakebolewski said he is working on it. Not sure about the status now.

@StefanKarpinski
Copy link
Member

Or just fix JuliaParser.

+1 to this reorg

andreasnoack added a commit that referenced this pull request Aug 2, 2015
[RFC] Moved linalg tests into better named files
@andreasnoack andreasnoack merged commit 2ef1362 into master Aug 2, 2015
@tkelman
Copy link
Contributor

tkelman commented Aug 2, 2015

we should wait until after coverage is working again

I guess @kshyatt should have titled this WIP if she didn't want it merged yet? Probably fine.

@tkelman tkelman deleted the ksh/reorganizelinalg branch August 2, 2015 17:46
@kshyatt
Copy link
Contributor Author

kshyatt commented Aug 2, 2015

I don't mind it being merged. If coverage goes down we can fix it. I think it will be much easier to diagnose coverage issues now.

@andreasnoack
Copy link
Member

Sorry about that. I missed the last part. I don't think it is worth reverting, but we should probably run compare coverage to the parent when JuliaParse is working again.

@ScottPJones
Copy link
Contributor

Any word from @jakebolewski on status? I'm tempted to look at it, if he's on vacation or something.

@@ -0,0 +1,105 @@
#This file is a part of Julia. License is MIT: http://julialang.org/license
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to start with # This file, the space between # and This is significant to the add_license_to_files.jl script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants