Skip to content

Conversation

@ben-schwen
Copy link
Member

Closes #3977

As pointed in the issue its faster (at least on my multi-core machine) to double call gsum due to #3202 than providing an implementation similar to e.g. gsd/gvar.

@ben-schwen ben-schwen marked this pull request as draft November 1, 2021 16:12
@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #5246 (fae5952) into master (2ef323c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5246   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files          77       77           
  Lines       14639    14645    +6     
=======================================
+ Hits        14567    14573    +6     
  Misses         72       72           
Impacted Files Coverage Δ
R/data.table.R 99.89% <100.00%> (+<0.01%) ⬆️

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 2ef323c...fae5952. Read the comment docs.

@ben-schwen ben-schwen marked this pull request as ready for review November 1, 2021 16:50
Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

Very good idea for implementation.

Not enough tests. Add some for NA, maybe NaN as well (both in x and w). Test against fixed values rather than stats function call, especially when object is so tiny. Add test where there is more than one group.

@mattdowle mattdowle added this to the 1.14.3 milestone Dec 3, 2021
@mattdowle mattdowle merged commit efee370 into master Dec 3, 2021
@mattdowle mattdowle deleted the gforce_weightedMean branch December 3, 2021 20:36
@jangorecki
Copy link
Member

mattdowle added a commit that referenced this pull request Dec 3, 2021
…h 'invalid first argument'; removing it passed all tests so it wasn't covered.
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GForce optimize weighted.mean

3 participants