-
Notifications
You must be signed in to change notification settings - Fork 2
Require undef in BlockSparseArray constructors #68
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
- Coverage 75.71% 75.46% -0.25%
==========================================
Files 29 28 -1
Lines 1083 1072 -11
==========================================
- Hits 820 809 -11
Misses 263 263
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@lkdvos I'd be curious to get your feedback on this, particularly the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall definitely looks good to me.
I left some comments about the organization of the constructor call chain, but keep in mind that I'm not sure if that structure is standard practice or simply something I have picked up over the years which I have found to be quite robust and stable.
Bottom line, this is a bit subjective anyways, but I do like the clear separation of responsibilities.
Considering the sparsemortar
name, I think that's good, and in line with sparserand
and sparsezeros
so that seems consistent.
I am wondering a bit if we can get away with not having that function at all, and repurposing sparse(mortar(args...))
instead, which is only a few characters longer and reuses functionality we would probably have to have anyways. (in which case we can still define it ofcourse). I don't think it matters too much of course, just wanted to share some thoughts 😉
@lkdvos in the latest I:
|
@lkdvos I've update the PR based on your last round of comments, this is good to go from my end. Let me know if you have any more comments. |
blocked axes `axes`. | ||
|
||
Note that `undef_blocks` is defined in | ||
[BlockArrays.jl](https://juliaarrays.github.io/BlockArrays.jl/stable/lib/public/#BlockArrays.undef_blocks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider re-exporting that too if it turns out we are using this often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that touches on a broader question I've been grappling with about how much we want to re-export things. So far my strategy with these new packages is to be very conservative about what we export, including not re-exporting anything and expecting users to load the packages where the objects originated themselves, but once the dust settles a bit and we start using these packages more I'm open to some targeted re-exports.
Fixes #69. Related to ITensor/SparseArraysBase.jl#33.
This also introduces
sparsemortar
, which is likeBlockArrays.mortar
but for constructing aBlockSparseArray
.