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

Update csr_sum.cc #7827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

sanowl
Copy link

@sanowl sanowl commented Oct 16, 2024

[$Feature] Improved Error Handling in CSR Sum Implementation

Description

This PR improves the error handling in the CSR Sum implementation. We've replaced assertion-style checks with more robust exception handling and added comprehensive input validation. This change enhances the reliability and debuggability of the CSR Sum function.

Checklist

  • The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
  • The PR is complete and small (less than 200 lines of core code change)
  • All changes have test coverage (Note: Tests need to be added)
  • Code is well-documented
  • To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change (Note: This needs verification)
  • Related issue is referred in this PR (Note: If there's a related issue, it should be referenced here)

Changes

  • Introduced custom CSRSumException for specific error handling
  • Replaced CHECK macros with explicit exception throwing
  • Added comprehensive input validation in CSRSum function
    • Empty input check
    • Matrix dimension consistency check
    • Data type and context consistency check
    • Array size validation
  • Improved error messages for better debugging
  • Updated documentation to reflect new error handling approach

Notes

  • Test coverage needs to be added to verify the new error handling mechanisms
  • The impact on existing examples and use cases should be assessed
  • Performance impact of the additional checks should be evaluated, though it's expected to be minimal

@dgl-bot
Copy link
Collaborator

dgl-bot commented Oct 16, 2024

Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:

  • @dgl-bot

@dgl-bot
Copy link
Collaborator

dgl-bot commented Oct 16, 2024

Commit ID: ed9664c

Build ID: 1

Status: ❌ CI test failed in Stage [Authentication].

Report path: link

Full logs path: link

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