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

get type from correct scope #223

Merged
merged 4 commits into from
May 10, 2022
Merged

get type from correct scope #223

merged 4 commits into from
May 10, 2022

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented May 9, 2022

Fixes #222

@sbfnk sbfnk requested a review from Bisaloo May 9, 2022 14:50
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #223 (b41f840) into master (ae816a7) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #223   +/-   ##
=======================================
  Coverage   91.21%   91.21%           
=======================================
  Files          22       22           
  Lines        1354     1354           
=======================================
  Hits         1235     1235           
  Misses        119      119           
Impacted Files Coverage Δ
R/utils_data_handling.R 92.53% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae816a7...b41f840. Read the comment docs.

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This fixed my use case. Shouldn't quantiles get the same treatment though (even though it's less likely to already have a column named quantiles in a sample based format)?

@sbfnk
Copy link
Contributor Author

sbfnk commented May 9, 2022

yes, good point

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We just need to add regression tests for this. I can do it if you'd like.

@sbfnk
Copy link
Contributor Author

sbfnk commented May 9, 2022

That would be great, thanks.

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It would be good to update the news with this fix and a link to this PR at the same time.

@Bisaloo
Copy link
Member

Bisaloo commented May 10, 2022

I'll let Nikos write the NEWS entry since I'm not sure if we're in time to make it into 1.0.0.

@nikosbosse nikosbosse merged commit 2ff6141 into master May 10, 2022
@nikosbosse nikosbosse deleted the fix-222 branch May 10, 2022 17:05
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.

sample_to_quantile() fails if one column is named type
4 participants