Skip to content

Conversation

dschult
Copy link

@dschult dschult commented Oct 25, 2023

Some misc fixes to remove remaining test failures for coo-1d.

  1. Mypy error on dtype not being defined in _spbase.
    We could set it to something expecting it to be overwritten by the subclass.
    But self.dtype.type appeared before this PR. Turns out adding -> str typing in the function signature line caused more strict checking by mypy. I removed the -> str thinking we can fix the mypy error on dtype in a different PR. If you prefer we can define it to be a vaccuous value that will be overwritten. I think with typing you can also set the type without assigning a value, but that choice is yours to make. :)

  2. Mypy error on max_size = np.prod(shape) because the result might not be an integer. I changed it to manually multiplying the values (which is 100 times faster anyway, but is very fast for both).

  3. den.resize(arg) was raising due to references of this array not allowing resize. So I added refcheck=False which was suggested by the test message. Also the reference is what we are creating: res and we know it isn't going to cause problems. [This occurs in 3 tests of resize within test_coo.py.]

  4. test error complaining about intc not equaling int32 when we used type(res) == type(exp). That idiom is used in a few other tests to check that they are both ndarrays. But in the 1d_mul_vector case, the result is a scalar. So the type becomes the dtype of the original arrays, and that can be intc for scipy and int32 for numpy. I don't think we need to test the type of the results. We should check it is a scalar and then check that the values are equal. Checking for scalar is suggested to be done (in the np.isscalar documentation) by testing that res.ndim == 0. So I replace the existing test with that test. The next line checks that res equals exp` so they both must be scalars.

  5. errors due to bounds exceeded within np.revel_multi_index inside _coo_base.reshape(). This had a TODO comment, that took me way too long to notice, pointing to a PR where the code to handle reshaping accounts for this limitation on revel_multi_index. The 1d case was actually easy to handle (index is already flat). So I added code to mimic what was there before (with indices instead of row and col). It is passing tests and hopefully OK.

  6. I removed white space on two empty lines in test_coo.py.

I think this could fix the failed tests in Linux Meson, Windows and Mypy. That should give us a green check if all goes well. Finger's crossed.

The upstream main can "fast-foward pull" after these changes except for some submodule updates. As far as I can tell that won't affect anything, but I don't know anything about submodule repos (except how to update them using git).

@perimosocordiae perimosocordiae merged commit 3133513 into perimosocordiae:coo-1d Nov 1, 2023
@perimosocordiae
Copy link
Owner

Thanks, Dan! Sorry this took me a while to get to.

@dschult
Copy link
Author

dschult commented Nov 1, 2023

Np… real life and all that.

Looks like the ravel/unravel still isn’t working on 32-bit machines.
I guess we need to use divmod as in: new_col, new_row = divmod(flat_indices, shape[0]) manually changing for order ‘F’ or ‘C’.

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