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

checkfunargs: use defaults, allow ellipses #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gkane26
Copy link

@gkane26 gkane26 commented Jan 28, 2022

Small edits to allow the use of ellipses in the objective function. I've used this a bit for personal use, but it hasn't undergone extensive testing.

Copy link
Owner

@astamm astamm 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 this PR. Can you please look into my comments?

@@ -313,16 +313,22 @@ function( x0,
if( any(is.na(m1)) ){
mx1 = which( is.na(m1) )
for( i in 1:length(mx1) ){
stop(paste(funname, " requires argument '", fnms[mx1[i]], "' but this has not been passed to the 'nloptr' function.\n", sep = ""))
has_default = !(fnms[mx1[i]] == "")
Copy link
Owner

@astamm astamm Jan 30, 2022

Choose a reason for hiding this comment

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

You should be checking the value in the list here, not the name, i.e. has_default = flist[[mx1[i] + 1]] != "".

for( i in 1:length(mx2) ){
stop(paste(rnms[mx2[i]], "' passed to (...) in 'nloptr' but this is not required in the ", funname, " function.\n", sep = ""))
if( !("..." %in% names(flist)) ) {
m2 = match(rnms, fnms)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you run again this? It is already in m1, isn't it?

}
}
m2 = match(rnms, fnms)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you run again this? It is already in m1, isn't it?

if( any(is.na(m2)) ){
mx2 = which( is.na(m2) )
for( i in 1:length(mx2) ){
stop(paste(rnms[mx2[i]], "' passed to (...) in 'nloptr' but this is not required in the ", funname, " function.\n", sep = ""))
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove parenthesis around ellipsis.

@astamm
Copy link
Owner

astamm commented Dec 22, 2022

Hi @gkane26. Thanks for the PR. I do not know whether you had a chance to see my comments and finalise the PR so that we could close the related issue. Let me know.

@bbolker
Copy link

bbolker commented Jun 24, 2024

Would you like me to take another crack at this?

It seems that this would be easier to implement by starting here and adding

flist <- formals(fun)                                ## existing code
flist <- flist[setdiff(names(flist), "...")]  ## new

although admittedly I haven't checked to see if there are any surprises down the line.

@aadler
Copy link
Contributor

aadler commented Jun 25, 2024

I’m not in front of my computer, but if I recall correctly, some of the helper wrapper functions use the ellipse to pass parameters to the underlying nloptr call, so ensuring that there is no conflict there would also be warranted.

@astamm
Copy link
Owner

astamm commented Jun 25, 2024

If you have time, that would be great.

@astamm
Copy link
Owner

astamm commented Jun 25, 2024

It seems related to #37 as well.

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.

4 participants