JuliaLinearAlgebra / MatrixDepot.jl

An Extensible Test Matrix Collection for Julia
http://matrixdepotjl.readthedocs.org/
Other
75 stars 22 forks source link

added support for all SuiteSparse metadata - improved testability - updated documentation #44

Closed KlausC closed 3 years ago

KlausC commented 4 years ago

This PR is a major rework in order to improve usability and testability of the package. The following features were added:

  1. Additional Metadata for the SuiteSparse problems are now available. That are all data which are described in SuiteSparse site. That includes for example posdef, a boolean indication for positive definite matrices, and the list of all singular values of the matrix with some indications derived from it, like cond, norm, rank. (Condition-number, 2-Norm, numerical rank wrt. a tolerance).

All these new attributes can be used to make selections of in the database.

  1. Speed-up testing by providing all required test data files in the project test directory. That avoids the need to download the files from the real web sites but allows a stable test environment. This behaviour is configurable by environment variable MATRIXDEPOT_URL_REDIRECT. Currently the "nightly-build" test runs load the test files from the remote sites, while the others use the stable test files.

  2. It is now possible to locate the local data cache according to environment variable MATRIXDEPOT_DATA. It used to be strictly in the package data subdirectory.

  3. A new package dependency on MAT is required to read ss_index.mat and the files containing the singular values. These files are in the MATLAB version 5 format.

  4. Open issues #29, #36, #40, and #45 are resolved with this PR.

  5. running on LInux/MacOS/Windows

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@423ba69). Click here to learn what that means. The diff coverage is 81.68%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #44   +/-   ##
=========================================
  Coverage          ?   89.45%           
=========================================
  Files             ?       14           
  Lines             ?     2362           
  Branches          ?        0           
=========================================
  Hits              ?     2113           
  Misses            ?      249           
  Partials          ?        0           
Impacted Files Coverage Δ
src/higham.jl 95.08% <ø> (ø)
src/markdown.jl 97.67% <0.00%> (ø)
src/downloadmm.jl 52.77% <52.77%> (ø)
src/data.jl 78.57% <81.81%> (ø)
src/download.jl 90.09% <90.00%> (ø)
src/downloadsp.jl 90.90% <90.90%> (ø)
src/matrixmarket.jl 91.66% <97.36%> (ø)
src/MatrixDepot.jl 96.15% <100.00%> (ø)
src/common.jl 97.23% <100.00%> (ø)
src/datareader.jl 87.91% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 423ba69...74d7b40. Read the comment docs.

KlausC commented 4 years ago

@andreasnoack , this PR is now in a state to be reviewed. All tests have succeeded. Following Issues have been resolved: #45, #42, #40, #38, #37, #36.

KlausC commented 4 years ago

bump @andreasnoack

andreasnoack commented 4 years ago

It makes it harder to review PRs when more than a single feature is added per PR and it seems to me that some of these features could be handled individually.

I don't like adding data files to repos if they are available from another "canonical" source. In https://github.com/JuliaMatrices/MatrixDepot.jl/pull/39, we discussed that the NIST server is unreliable and my preferred solution was just to drop matrix marked since it's obsolete.

While checking in the data files might save a little time if the developer runs all tests locally, I don't think it matters much for CI. It just moves the download from one server to another.

KlausC commented 4 years ago

Hi Andreas, what do you propose me to to?

I thought I was working in the sense of #39 with this major work. I was failing to understand a clear directive to drop support for NIST Matrix Market.

The local testability was a feature from the retired #38, which never got a negative review. I still think it is useful to have a complete set of test data in the repository, to be independent from the third party resource, even if it is canonical. The main reason for that was, that the CI environments for Windows failed, when trying the remote access in general. Instead of generating own test data files, I preferred to copy some of the available canonical files. Your are right about there are quite some features mixed into this PR. But essentially is the added functionality with regard to the singular values, which has been requested in several issues. Others are minor bug fixes. The documentation extensions should not be separated from the features, which have been added.

andreasnoack commented 4 years ago

Don't get me wrong. It might be that all the features and fixes should go into the repo. It's just much easier to review when split into minimal pieces. Sometimes, large diffs are needed but from your list, it looks like most of them could be handled separately. It also makes it possible to merge uncontroversial parts immediately while discussing other parts.

That being said, you are currently the only active contributor and you do some good work here so ultimately feel free to decide if you'd like to proceed with this as is. I might be able to review if you split this PR up into smaller pieces but I probably won't be able to make it in its current form.

andreasnoack commented 3 years ago

@KlausC I put the 0.8.1 release on pause but should we go ahead with it and then merge this one and make a 0.9?

I hit an issue today where I couldn't load cached matrices after restarting my session. Is that something you've hit and does this one address it?

KlausC commented 3 years ago

@andreasnoack, I think I remember, that I fixed an issue like you described.

I am currently testing on juliav1.6, and found an incompatibility of download. Want to fix that, and then merge. https://github.com/JuliaLang/julia/issues/38525

What about making release 1.0?

andreasnoack commented 3 years ago

1.0 sounds good

andreasnoack commented 3 years ago

Is this one ready?

KlausC commented 3 years ago

Just tested on windows locally. Want to add minor changes, then merge this evening.

KlausC commented 3 years ago

I see - those entries have been proposed by this compatibility bot. I will push another commit with changed entries.