Skip to content

simplify the normalization in gpdfit#187

Merged
jgabry merged 2 commits into
masterfrom
avoid-unnecessary-computation-in-gpdfit
Oct 7, 2021
Merged

simplify the normalization in gpdfit#187
jgabry merged 2 commits into
masterfrom
avoid-unnecessary-computation-in-gpdfit

Conversation

@avehtari

Copy link
Copy Markdown
Member

Seth Axen pointed out that

w_theta <- 1 / vapply(jj, FUN.VALUE = numeric(1), FUN = function(j) {
  sum(exp(l_theta - l_theta[j]))
})

Is doing normalization in O(n^2), while it can be simplified to O(n) and also made more stable as

w_theta <- exp(l_theta - matrixStats::logSumExp(l_theta))

@ParadaCarleton

ParadaCarleton commented Sep 20, 2021

Copy link
Copy Markdown

@sethaxen Is this taken from some of the changes I made while writing ParetoSmooth.jl, or is it a separate performance improvement I should add to ParetoSmooth myself? (I keep forgetting what all the weird little apply functions in R do.)

@sethaxen

Copy link
Copy Markdown

@ParadaCarleton no, this isn't from ParetoSmooth.jl. If the improvement isn't in there, it should be.

@jgabry

jgabry commented Sep 20, 2021

Copy link
Copy Markdown
Member

This looks good, thanks! I need to investigate why the Windows checks are failing but haven't had a chance yet. I'm guessing it's unrelated to this PR, but want to double check before merging.

@jgabry

jgabry commented Sep 21, 2021

Copy link
Copy Markdown
Member

The windows test failure is happening here:

test_that("loo.array runs with multiple cores", {
loo_with_arr1 <- loo(LLarr, cores = 1, r_eff = NA)
loo_with_arr2 <- loo(LLarr, cores = 2, r_eff = NA)
expect_identical(loo_with_arr1$estimates, loo_with_arr2$estimates)
})

the error message in the github actions log says that the error occurs because the objects are "equal but not identical". This usually means that the value is the same but some attribute differs, but I can't think of any reason how this PR could cause that.

Does anyone have access to a windows machine? If so could you try running this one test locally?

@codecov-commenter

codecov-commenter commented Sep 21, 2021

Copy link
Copy Markdown

Codecov Report

Merging #187 (769f34f) into master (c91f8be) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 769f34f differs from pull request most recent head 6b87ccb. Consider uploading reports for the commit 6b87ccb to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   95.15%   95.15%   -0.01%     
==========================================
  Files          28       28              
  Lines        2663     2662       -1     
==========================================
- Hits         2534     2533       -1     
  Misses        129      129              
Impacted Files Coverage Δ
R/gpdfit.R 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 c91f8be...6b87ccb. Read the comment docs.

@jgabry

jgabry commented Sep 23, 2021

Copy link
Copy Markdown
Member

reminder to self: @mike-lawrence nicely offered remote access to a windows machine, so remember to take him up on that offer

@jgabry

jgabry commented Oct 7, 2021

Copy link
Copy Markdown
Member

Ok so it's now passing on Windows despite us not changing anything. The only thing I did was wait a few weeks and rerun the github actions workflow. Anyway, I think we can go ahead and merge this.

@jgabry jgabry merged commit 314db55 into master Oct 7, 2021
@jgabry jgabry deleted the avoid-unnecessary-computation-in-gpdfit branch October 7, 2021 00:40
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.

5 participants