swirldev / swirl

:cyclone: Learn R, in R.
http://swirlstats.com
Other
1.13k stars 593 forks source link

Issue with named arguments in functions [Results is "wrong" answer] #182

Closed SplashDance closed 9 years ago

SplashDance commented 10 years ago

There seems to be an issue with passing named arguments in functions during the swirl exercises.

For example, in the Regression Models track, entering a formula such as var(est(slope = ols.slope, intercept = ols.ic)) will result in a message such as That's not the answer I was looking for, but try again. whereas eliminating the named arguments: var(est(ols.slope, ols.ic)) will be accepted as correct.

ncarchedi commented 10 years ago

That's correct. I've addressed this in the new content under development, but haven't had a chance to adjust the other content.

@wilcrofter could you take a look at my match_call custom test in Getting and Cleaning Data (swirl_misc) and let me know what you think about implementing something similar in omnitest or as a standalone test?

reginaastri commented 10 years ago

match_call is better than what we use in omnitest now to compare expressions. It works very well, however there's a possible problem exemplified by this:

> q1 <- expand_call("var(y=a,x=b,FALSE)")
> q2 <- expand_call("var(y=a,x=b)")
> identical(q1,q2)
[1] FALSE
> 
> q1
var(x = b, y = a, na.rm = FALSE)
> q2
var(x = b, y = a)
> 
WilCrofter commented 10 years ago

I'd vote for a standalone test which is used by omnitest (and perhaps others) internally. Good show.

ncarchedi commented 10 years ago

@reginaastri I see what you mean. The underlying match.call() function seems to only be capable of handling arguments that are specified. In your example, q1 and q2 are identical, but q2 does not (redundantly) specify the value of na.rm, instead allowing it to assume its default value of FALSE.

It's possible we could use formals() to match in arguments that are unspecified. For example, try formals(var).

ncarchedi commented 10 years ago

I plan to work on this later in the week. I'll include something like my custom test match_call() as a standalone test in answerTests2.R.

@reginaastri Regarding matching unspecified arguments, we may have to fight that battle another day, as it's not clear to me the best way to go about it.

SplashDance commented 10 years ago

@ncarchedi : As an aside, should we instead be using the coef() function to access coefficients instead of accessing them directly (via the $ operator)? Or, at the very least, have you thought about adding solutions using coef() as possible (correct) answers? [Note: it's the questions dealing with the 'swiss' dataset that got me thinking about this...]

WilCrofter commented 10 years ago

I may have written the lessons in question. It is clear they could and should be improved as time and effort allow. The associated Coursera material, however, is undergoing revision, and we've been advised to wait for that effort to progress before doing anything major.