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

Pja/externalize custom generators #79

Merged

Conversation

willow-ahrens
Copy link
Contributor

Re-requesting willow-ahrens#1 directly to MatrixDepot...

This change would deprecate the custom code loading in MatrixDepot. In particular, this makes three key changes:

  1. MatrixDepot will no longer create or write to MYDEPOT. While MatrixDepot still reads these files, it will issue a deprecation warning if they are not the default files.
  2. MatrixDepot no longer serializes user-defined generators because they may or may not be loaded when the database is deserialized
  3. The Documentation now explains the replacement functionality, which directs users to call include_generator from outside of MatrixDepot at runtime, then reinitialize.

Note that we cannot remove the code associated with determining the path of MYDEPOT until we move from deprecation to removal at some point in the future.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #79 (533f61e) into master (d1788e7) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   66.12%   66.20%   +0.08%     
==========================================
  Files          14       14              
  Lines        2456     2459       +3     
==========================================
+ Hits         1624     1628       +4     
+ Misses        832      831       -1     
Impacted Files Coverage Δ
src/MatrixDepot.jl 95.83% <100.00%> (-0.47%) ⬇️
src/common.jl 46.33% <100.00%> (+0.80%) ⬆️
src/download.jl 89.85% <100.00%> (+0.14%) ⬆️
src/markdown.jl 21.11% <100.00%> (ø)
src/types.jl 45.60% <100.00%> (+0.43%) ⬆️

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 d1788e7...533f61e. Read the comment docs.

Copy link
Collaborator

@KlausC KlausC left a comment

Choose a reason for hiding this comment

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

In general, you are deprecating existing functionality, which you don't need.
You show, that is possible, as a replacement, to inject function defined in an own module/package, which is using MatrixDepot. In order to make that happen, a synchronisation bug of the serialized database was fixed.
TBH, I don't see the advantages of removing the existing functionality.

I would be happy with the deprecation, though, if we could present and document a complete replacement for the extensability. That means, we provide a sample extension package, which contains the function definitions, and which adds those to the MatrixDepot database with all data, we have with the old solution.

doc/user.rst Outdated Show resolved Hide resolved
doc/user.rst Outdated Show resolved Hide resolved
doc/user.rst Outdated Show resolved Hide resolved
doc/user.rst Outdated Show resolved Hide resolved
src/MatrixDepot.jl Show resolved Hide resolved
src/MatrixDepot.jl Outdated Show resolved Hide resolved
src/MatrixDepot.jl Outdated Show resolved Hide resolved
src/download.jl Show resolved Hide resolved
test/include_generator.jl Show resolved Hide resolved
test/include_generator.jl Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
doc/user.rst Outdated Show resolved Hide resolved
@KlausC
Copy link
Collaborator

KlausC commented Jun 12, 2021

I made a template package to demonstrate what I mean: MatrixDepotTemplate

@willow-ahrens
Copy link
Contributor Author

willow-ahrens commented Sep 19, 2021

Apologies that this took me so long to get back to, I needed a decent chunk of time to address these changes.

To address why this PR is useful, suppose a user wants to generate some matrices defined in a repo on the internet.

In the old MatrixDepot, they would have to check out the MatrixDepot project for development Pkg.dev and copy that code into the MYDEPOT dir. If they copy it without doing Pkg.dev first, they might lose the code when they update MatrixDepot. If the local MatrixDepot is ever deleted, they might lose that code. To get around this, they could also specify an environment variable every time they run Julia to say where their custom user-generated depot code is loaded. They would need to copy new versions of the external generator code every time it is updated.

In the new MatrixDepot, the user only needs to type e.g. using MatrixDepotTemplate. The package manager handles new versions of matrix generation code, just like how code loading is handled everywhere else in Julia. The user can share their code with others, and it will work without the others needing to set environment variables or manually copy code.

With this PR, the following code works out of the box:

julia> using MatrixDepot
[ Info: verify download of index files...
[ Info: reading database
[ Info: adding metadata...
[ Info: adding svd data...
[ Info: writing database
[ Info: used remote sites are sparse.tamu.edu with MAT index and math.nist.gov with HTML index

julia> using MatrixDepotTemplate

julia> mdinfo("randsym")
  random symmetric matrix (randsym)
  ≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡

  Input options:

    •  n: the dimension of the matrix

@willow-ahrens
Copy link
Contributor Author

willow-ahrens commented Sep 19, 2021

In short, this PR empowers users to define custom suites of matrices that can be loaded as packages, maintaining the same MatrixDepot interface.

@willow-ahrens
Copy link
Contributor Author

I believe that I have implemented all of the requested changes. I'm not sure why github is showing that 1 change is still requested. I also merged master into this branch and updated some of the logging-related tests to be aware of MatrixDepot now using @info.

@willow-ahrens willow-ahrens requested a review from KlausC January 5, 2022 22:16
@willow-ahrens
Copy link
Contributor Author

willow-ahrens commented Jan 5, 2022

The two tests that fail on nightly also fail on master. I'm not sure what's the problem with them, but it's fairly clear to me that they are unrelated to the changes I've made in this PR. I think they have something to do with the gzip utility.

@willow-ahrens
Copy link
Contributor Author

When I say that those tests fail on master, I mean that MatrixDepot/master is failing on julia nightly with the same errors. I'll file a separate issue.

Copy link
Collaborator

@KlausC KlausC left a comment

Choose a reason for hiding this comment

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

That looks good to me, now. The pending issue has been resolved in the meanwhile.

@willow-ahrens
Copy link
Contributor Author

Yep! Merging master into this branch appears to have resolved those tests. This looks ready to me as well.

@KlausC KlausC merged commit b08778e into JuliaLinearAlgebra:master Jan 11, 2022
@KlausC
Copy link
Collaborator

KlausC commented Jan 13, 2022

@peterahrens , thanks for your very welcome contributions. Release v1.0.6 and newer should be close to your requirements now.

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.

2 participants