GeezerVison

coverage: 99.396% (-0.08%) from 99.48%
when pulling b4a80a4 on eriknw:from_dense
into 1bbce69 on python-graphblas:main.

missing_value= is a nice addition to from_dense. However, missing_value= doesn't make any sense for building a dense object from a scalar, so I feel like this is now squeezing too much functionality into from_dense.

Compare:

def from_dense(cls, value, missing_value=None, dtype=None, *, nrows=None, ncols=None, name=None):
    # handle scalars and arrays

vs

    def from_dense_scalar(cls, value, nrows, ncols, dtype=None, *, name=None):
    def from_dense_array(cls, values, missing_value=None, dtype=None, *, name=None):

I prefer two methods. But, what should we name them?

I prefer two methods. But, what should we name them?

Perhaps:

• from_dense for arrays
• Add fill_value=None to the main constructor to fill with a scalar
  • e.g., Vector(size=5, fill_value=10)
• or from_iso_value for scalar

Splitting from_dense into two functions (one for "dense from scalar") simplified the implementation, so +1 from me.

I still need to update documentation. Do we like from_iso_value?

Splitting from_dense into two functions (one for "dense from scalar") simplified the implementation, so +1 from me.

Yes, it is more readable now.

I still need to update documentation. Do we like from_iso_value?

Looks good, I'm not sure how frequently this specific function is used (e.g. in algorithms).

Updated and ready for review.

We discussed this in our meeting today, and we settled on from_scalar instead of from_iso_value. We should also add a couple more notes such as this being cheap / low storage in SuiteSparse:GraphBLAS, and maybe show an alternative example for how to create a new matrix with a scalar value and a mask.

We could consider adding a mask= keyword to from_scalar, but this implies a shape, which makes things a little awkward.

from_iso_value is renamed to from_scalar, and I made a note about SuiteSparse:GraphBLAS being efficient.

@jim22k, what note about masks did you want to add?

I think this PR is ready to merge, but I'll wait for an approval (for a couple days).

The note could say that if you want to create an iso-valued Matrix or Vector with the same structure as an existing object, use obj.apply(binary.second, right=scalar).

Thanks @jim22k and @SultanOrazbayev for your input on this PR! Even though I'm getting credit for commits, I know y'all deserve credit for helping too, because your engagement does make things better ❤️

This is going in! 🚀