Skip to content

gmin/gmax properly support integer64#4445

Merged
mattdowle merged 7 commits intomasterfrom
gmin-bit64
Aug 20, 2021
Merged

gmin/gmax properly support integer64#4445
mattdowle merged 7 commits intomasterfrom
gmin-bit64

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented May 14, 2020

Closes #4444

MichaelChirico
MichaelChirico commented May 14, 2020
View reviewed changes
Comment thread src/gsumm.c Outdated
@codecov
Copy link
Copy Markdown

codecov bot commented May 14, 2020 •
edited
Loading

Codecov Report

Merging #4445 (015e3b8) into master (4d68901) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4445   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          76       76           
  Lines       14592    14611   +19     
=======================================
+ Hits        14525    14544   +19     
  Misses         67       67
Impacted Files Coverage Δ
src/gsumm.c 100.00% <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 4d68901...015e3b8. Read the comment docs.

MichaelChirico
MichaelChirico commented May 14, 2020
View reviewed changes
Comment thread src/gsumm.c Outdated
@@ -896,39 +930,62 @@ SEXP gmax(SEXP x, SEXP narm)
break;
case REALSXP:
Copy link
Copy Markdown
Member Author

@MichaelChirico MichaelChirico May 14, 2020

Choose a reason for hiding this comment

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

There's a TODO above:

// TODO rework gmax in the same way as gmin and remove this *update

I partially addressed that in this PR, only for the REALSXP case

jangorecki
jangorecki reviewed May 9, 2021
View reviewed changes
Comment thread inst/tests/tests.Rraw Outdated
Comment on lines +17449 to +17450
i64_default_min = suppressWarnings(min(as.integer64(NA), na.rm=TRUE))
i64_default_max = suppressWarnings(max(as.integer64(NA), na.rm=TRUE))
Copy link
Copy Markdown
Member

@jangorecki jangorecki May 9, 2021

Choose a reason for hiding this comment

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

I would add tests for that as well so in case bit64 will change this behavior we will be notified

Copy link
Copy Markdown
Member Author

@MichaelChirico MichaelChirico May 10, 2021

Choose a reason for hiding this comment

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

good idea. actually it turns out they aren't warning, which differs from the equivalent base behavior. So the test is a bit wonky, WDYT?

jangorecki
jangorecki reviewed May 10, 2021
View reviewed changes
Comment thread NEWS.md Outdated
Comment on lines 89 to 91
Copy link
Copy Markdown
Member

@jangorecki jangorecki May 10, 2021

Choose a reason for hiding this comment

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

@MichaelChirico

@mattdowle mattdowle mentioned this pull request Aug 20, 2021
Merged
5 tasks
@mattdowle mattdowle added this to the 1.14.1 milestone Aug 20, 2021
@mattdowle mattdowle merged commit 01da119 into master Aug 20, 2021
@mattdowle mattdowle deleted the gmin-bit64 branch August 20, 2021 06:51
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jangorecki jangorecki jangorecki left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Not ignoring NAs with 64-bit integers and grouped operations?

3 participants

@MichaelChirico @jangorecki @mattdowle