Skip to content

updated new functions and tests#7

Closed
Rutgervdspek wants to merge 1 commit intoAlexisDerumigny:mainfrom
Rutgervdspek:feature/new_feature
Closed

updated new functions and tests#7
Rutgervdspek wants to merge 1 commit intoAlexisDerumigny:mainfrom
Rutgervdspek:feature/new_feature

Conversation

@Rutgervdspek
Copy link
Contributor

No description provided.

Copy link
Owner

@AlexisDerumigny AlexisDerumigny left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, very nice! I have a few comments that I put in as well.
Could you do the changes, and before this, could you also merge the current main branch in your branch so as to avoid the current merge conflict? Then you can update this PR.

If this is not clear, please feel free to let me know

* New functions:

- `CondMeanEst`: nonparametric estimation of the conditional mean.
(#2, thanks to Rutger van der Spek)
Copy link
Owner

Choose a reason for hiding this comment

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

Here, the numbering of the PR is off: you mention 2 but it should be #6 and this one #7

^ (d/2) ) ^ (2/d) )
if (dopb){ pbapply::setpb(pb, i) }
for (i in 1:n) {
x_quad = Rmpfr::mpfr(as.numeric((X[i,] - mu) %*% Sigma_m1 %*% (X[i,] - mu)),
Copy link
Owner

Choose a reason for hiding this comment

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

Here I think you can just write
x_quad = Rmpfr::mpfr( (X[i,] - mu) %*% Sigma_m1 %*% (X[i,] - mu),
so without the as.numeric

gridZ = seq(0, 1, length.out = 4)
h = 0.5

est = CondCovEst(X, Z, gridZ, h)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you update this test with the three different types of estimators?

expect_equal(dim(gEst), c(length(grid), length(gridZ)))
})

test_that("CondEllGenEst produces finite, non-negative values for simple Gaussian data", {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a test that the mpfr version gives similar results in this simple case?


mu = rbind(gridZ, gridZ)
sigma = array(0, dim = c(2,2,2))
sigma[,,] = diag(2)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you also add one more test where the covariance matrix is not the identity? Just to test whether this work also (this is also linked with the standardization of the generator that we discussed, in relation with commit fae2f25 )

@Rutgervdspek
Copy link
Contributor Author

Rutgervdspek commented Nov 24, 2025 via email

@AlexisDerumigny
Copy link
Owner

Hi Rutger,

Thanks for the update. The GitHub checks are done on several machines and OS.
I have just had a look at your new PR #9.

I see that the macOS check fails with the error Error: The template is not valid. r-lib/actions/v2/setup-r-dependencies/action.yaml (Line: 204, Col: 16): hashFiles('./.github/pkg.lock') failed. Fail to hash files under directory '/Users/runner/work/ElliptCopulas/ElliptCopulas'
so this is a bug at the level of the GitHub actions themselves, so you don't need to worry about them: I will have a look this myself later to see how to fix this (probably I need to update the checks to the latest version).

Both the Windows and Linux checks fails because of a warning:

❯ checking Rd \usage sections ... WARNING
  Undocumented arguments in Rd file 'CondEllGenEst.Rd'
    'mpfr' 'precBits' 'dopb'

I have seen that you have added the documentation for these arguments in the R file using the template, which is good. But they are not in the corresponding .Rd file. so probably you have forgotten to run devtools::document().
Once you will have done this, you can update #9 (you don't need to start a new pull request each time, you can just continue updating the previous one).
Then this should pass all the checks on Windows and Linux.

Best,
Alexis

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.

2 participants