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

MIT-licensed sparse() parent method and expert driver, take two #15242

Merged
merged 1 commit into from
Feb 26, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Feb 25, 2016

See #14798 (the original pull request) for discussion. Apologies for the close/reopen-glitch!

@tkelman tkelman added sparse Sparse arrays potential benchmark Could make a good benchmark in BaseBenchmarks labels Feb 25, 2016
@@ -91,6 +91,8 @@ Library improvements
* Rank one update and downdate functions, `lowrankupdate`, `lowrankupdate!`, `lowrankdowndate`,
and `lowrankdowndate!`, for dense Cholesky factorizations ([#14243],[#14424])

* All `sparse` methods now retain provided numerical zeros as structural nonzeros.
Copy link
Contributor

Choose a reason for hiding this comment

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

mention SparseArrays.dropzeros! as the way to get rid of them when the user would like to do so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds wise. In fact, given dropzeros! should become increasingly important / regularly used with the shift to retaining numerical zeros by default, perhaps dropzeros! should now be exported?

Copy link
Member

Choose a reason for hiding this comment

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

+1 for exporting dropzeros! and making other operations retain zeros (including the sparse function).

@Sacha0
Copy link
Member Author

Sacha0 commented Feb 26, 2016

mention SparseArrays.dropzeros! as the way to get rid of them when the user would like to do so?

I added lines to this effect to sparse's documentation and NEWS.md (without the SparseArrays qualification, anticipating export of dropzeros!).

Assuming exporting dropzeros! seems advantageous to all thread participants, should I add a separate commit to this PR exporting dropzeros!? Or would opening a separate PR (to broaden the change's visibility and hence opportunity for comment) be better?

@tkelman
Copy link
Contributor

tkelman commented Feb 26, 2016

Maybe do the export separately, I'd rather get this merged. Hopefully @jrevels and others will figure out the right way to standardize running package-based benchmarks over time.

When it does get exported, it should also be added to the docs and probably tested a bit more extensively.

@Sacha0
Copy link
Member Author

Sacha0 commented Feb 26, 2016

Maybe do the export separately, I'd rather get this merged. Hopefully @jrevels and others will figure out the right way to standardize running package-based benchmarks over time.

When it does get exported, it should also be added to the docs and probably tested a bit more extensively.

Sounds great. Let me know if any other touch-ups are in order here. Thanks again, and best!

@tkelman
Copy link
Contributor

tkelman commented Feb 26, 2016

apt-get travis issue addressed by #15249, restarted

dpkg: dependency problems prevent configuration of gfortran-5:i386:
 gfortran-5:i386 depends on gcc-5 (= 5.2.1-23ubuntu1~12.04); however:
  Package gcc-5:i386 is not configured yet.
dpkg: error processing gfortran-5:i386 (--configure):
 dependency problems - leaving unconfigured
Setting up libdpkg-perl (1.16.1.2ubuntu7.7) ...
Setting up zlib1g-dev:i386 (1:1.2.3.4.dfsg-3ubuntu4) ...
Setting up libssl-dev:i386 (1.0.1-4ubuntu5.34) ...
Setting up make:i386 (3.81-8.1ubuntu1.1) ...
Processing triggers for libc-bin ...
ldconfig deferred processing now taking place
Errors were encountered while processing:
 gcc-5:i386
 g++-5:i386
 gfortran-5:i386
E: Sub-process /usr/bin/dpkg returned an error code (1)

edit: since that was fixed by a .travis.yml change, it may need a rebase to actually apply on the PR build. This passes on win32 so I don't see why it wouldn't pass on linux 32 bit.

tkelman added a commit that referenced this pull request Feb 26, 2016
MIT-licensed sparse() parent method and expert driver, take two
@tkelman tkelman merged commit 026fa10 into JuliaLang:master Feb 26, 2016
@Sacha0
Copy link
Member Author

Sacha0 commented Feb 26, 2016

@tkelman Much thanks for the thorough review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
potential benchmark Could make a good benchmark in BaseBenchmarks sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants