Skip to content

Assorted fixes/improvements#3

Open
thchr wants to merge 5 commits intowildart:masterfrom
thchr:master
Open

Assorted fixes/improvements#3
thchr wants to merge 5 commits intowildart:masterfrom
thchr:master

Conversation

@thchr
Copy link
Copy Markdown

@thchr thchr commented Mar 9, 2021

Hi @wildart

I went ahead and vendored this package as a git subtree in my own package.
In doing that, I had a quick look at the code and wanted to change a few things: I'm not sure if you're interested in me upstreaming anything, but in case you were, I figured I might as well submit a PR (if you are not interested in maintaining this much, PRs etc., no problem - I just wanted to submit it in case you were).

The main things this does is:

  • Previously, the SNF field of Smith was an AbstractVector{P}; I added a V parametric type to Smith so that this could be concretely typed instead.
  • A few simplifications, e.g. in diagm.
  • Slightly more info when printing a Smith struct, and also overload show(::IO, ::MIME"text/plain", ...) instead of just show(::IO, ...).
  • A doc string for smith.
  • Some whitespace/alignment cleanup.

I had originally wanted to remove the dependency on SparseArrays, but it seems that it is not so easy to do generic programming with sparse vs. dense matrices as I had thought.

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 9, 2021

Coverage Status

Coverage decreased (-2.4%) to 92.67% when pulling b752789 on thchr:master into 0b61553 on wildart:master.

@thchr
Copy link
Copy Markdown
Author

thchr commented Mar 9, 2021

There was a test failure on v1.1 due to diagm and spdiagm apparently missing some methods there, relative to more recent versions; just rolled back to what you had before there then.

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