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

ccs_accum_nbad type mismatch #6

Closed
moocow-the-bovine opened this issue Apr 16, 2022 · 4 comments
Closed

ccs_accum_nbad type mismatch #6

moocow-the-bovine opened this issue Apr 16, 2022 · 4 comments

Comments

@moocow-the-bovine
Copy link
Owner

moocow-the-bovine commented Apr 16, 2022

see #5 (comment)

  • ccs_accum_nbad should get out_type => 'indx' rather than out_type => 'int+'
  • tests for ccs_accum_nbad should ensure $missing->type == $nzvalsIn->type
  • CCS fix should wait until PDL segfaults with current behavior have been dealt with
@moocow-the-bovine moocow-the-bovine mentioned this issue Apr 16, 2022
@mohawk2
Copy link
Collaborator

mohawk2 commented Apr 16, 2022

Thank you for your extremely thorough attention to this fascinating, subtle problem!

For the high-level PDL::CCS::Nd wrappers, I think it's always the case that NNzOut==NnzIn, so it doesn't make sense there. I can imagine cases with structured data where it does make sense though, and it could be wasteful (especially for ufuncs) to force NNzOut==NnzIn there, so I'm inclined to leave those dimensions distinct.

My recommendations are based on what I've learned from investigating this, and also from my recent experience of using a lot of RedoDimsCode in PDL::LinearAlgebra so I could just pass in null instead of the agonising nonsense with zeroes. They are (I removed the ones you've indicated above since you obviously know your library better than I!):

I believe with those points, you could heavily reduce the PMCode (though not eliminate thanks to the slicing requirement).

@moocow-the-bovine
Copy link
Owner Author

moocow-the-bovine commented Apr 19, 2022

  • "easy" (= symptomatic) fixes applied in 4b653fe , uploaded to CPAN as PDL::CCS v1.23.20
  • badflag-handling for ufunc counter-functions (nbad, ngood, nnz) adjusted too (but probably not sufficiently)
  • broadcastloop and $PDLSETSTATE*(ndarrayname) in Code appear not to work with my (probably outdated) PDL-2.019 (-> yes, I should upgrade, but am not going to do that right now)
  • signature-altering changes, moving dim-checks to RedoDimsCode (in a way which is compatible with older PDLs), and minimizing/eliminating PMCode are all out of my (TUIT) price range at the moment

... leaving this issue open for now, although I may copy+paste your suggestions (for which many thanks, in particular for the linked examples!) into separate issues later.

@mohawk2
Copy link
Collaborator

mohawk2 commented Nov 14, 2024

Switching the minimum PDL 2.081, as discussed on #13, will allow using indx where appropriate, which will solve the type mismatch issue.

@moocow-the-bovine
Copy link
Owner Author

... leaving this issue open for now, although I may copy+paste your suggestions (for which many thanks, in particular for the linked examples!) into separate issues later.

@mohawk2
I've made #16 for the outstanding overhaul issues. I think that with your recent #15 and 2ad95d3 , this issue can be closed. Feel free to re-open if you disagree.

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

No branches or pull requests

2 participants