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.

Sign up for GitHub

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

Jump to bottom

nvcomp tests: update hardcoded lengths #156

Merged
merged 4 commits into from
Jan 26, 2023
Merged

nvcomp tests: update hardcoded lengths #156

merged 4 commits into from
Jan 26, 2023

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Jan 23, 2023

@thomcom can we avoid hard coding the expected compression lengths?

@madsbk madsbk added bug Something isn't working non-breaking Introduces a non-breaking change labels Jan 23, 2023
@madsbk madsbk marked this pull request as ready for review January 23, 2023 10:20
@madsbk madsbk mentioned this pull request Jan 23, 2023
Merged
@thomcom
Copy link
Contributor

thomcom commented Jan 23, 2023

Seems like we could expect a range, or a maximum, or just not test them.

  1. Testing a range is like having hardcoded values with a lower frequency of changes required
  2. Testing a maximum allows the results to improve in any way but makes us do leg work if the upstream team produces something with lower quality, potentially allowing us to flag it for them
  3. Not testing the results leaves it in the hands of the NVCOMP team.

What do you suggest?

@madsbk
Copy link
Member Author

madsbk commented Jan 24, 2023

Thanks @thomcom, we are now using np.testing.assert_allclose(actual, desired, rtol=0.1) when comparing sizes. I think that is sufficient for now.

@madsbk madsbk requested a review from thomcom January 24, 2023 07:34
@bdice
Copy link
Contributor

bdice commented Jan 25, 2023

Can this PR move forward? It is blocking #154 which in turn blocks GitHub Actions migrations. cc: @ajschmidt8

@madsbk madsbk mentioned this pull request Jan 26, 2023
Merged
@quasiben
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit a35d9e7 into rapidsai:branch-23.02 Jan 26, 2023
@madsbk madsbk deleted the nvcomp_tests branch January 30, 2023 09:51
@madsbk madsbk mentioned this pull request Feb 6, 2023
Merged
rapids-bot bot pushed a commit that referenced this pull request Feb 7, 2023
@madsbk
nvcomp xfail compression ratios (#167)
16c61d8 
Mismatch in compression ratios now results in a `XFAIL`. 

In #156 we allowed a small compression ratio mismatch however this isn't enough when testing on ARM. 

cc. @thomcom @AjayThorve

Authors:
  - Mads R. B. Kristensen (https://github.com/madsbk)

Approvers:
  - Benjamin Zaitlen (https://github.com/quasiben)

URL: #167
vuule pushed a commit to vuule/kvikio that referenced this pull request Nov 8, 2023
@mroeschke
Return correct index when loc.__getitem__[scalar] with CategoricalInd…
d57d707 
…ex (rapidsai#156)

Before, this would return an Index of the same type of the Categorical's sub type.

I think long term it would be great to translate loc indexing in terms of iloc indexing (IIRC that's what pandas tries to do for a lot of cases)

Authors:
  - Matthew Roeschke (https://github.com/mroeschke)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: rapidsai/cudf-private#156
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quasiben quasiben quasiben approved these changes

@thomcom thomcom Awaiting requested review from thomcom

Assignees
No one assigned
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@madsbk @thomcom @bdice @quasiben