webppl icon indicating copy to clipboard operation
webppl copied to clipboard

Make method argument to Infer case-insensitive?

Open longouyang opened this issue 8 years ago • 8 comments

I often forget that the acronyms are capitalized (SMC) but the non-acronyms aren't (rejection). Can we just ignore case for the method property?

longouyang avatar Sep 16 '16 20:09 longouyang

I think this would be fine.

null-a avatar Sep 16 '16 20:09 null-a

Can we decide on a general strategy for string options, not introduce a special case for method? This seems like a good idea, since we aren't entirely consistent at the moment. Consider Optimize(model, {optMethod: 'sgd'}); vs Optimize(model, {estimator: 'ELBO'});.

stuhlmueller avatar Sep 16 '16 22:09 stuhlmueller

How about a global policy of being case-insensitive?

longouyang avatar Sep 16 '16 22:09 longouyang

Some of our values can be used as keys as well, e.g.

`Optimize(model, {optMethod: 'sgd'});`

and

`Optimize(model, {optMethod: {sgd: {stepSize: 0.5}}});`

Would we allow case-insensitive keys as well? The current choice of capitalization for method resulted from using the same approach that we're using for keys (camelCase, with acronyms capitalized).

If we don't allow case-insensitive arguments, we could also just print more informative error messages. ("Did you mean 'SMC'?")

stuhlmueller avatar Sep 16 '16 22:09 stuhlmueller

Would we allow case-insensitive keys as well? The current choice of capitalization for method resulted from using the same approach that we're using for keys (camelCase, with acronyms capitalized).

Yes?

longouyang avatar Sep 16 '16 22:09 longouyang

This feels pretty radical. Personally, I'd choose consistency over convenience here, and improve error messages if necessary. But if Paul is ok with making everything case-insensitive, I'll be ok with it, too.

stuhlmueller avatar Sep 16 '16 22:09 stuhlmueller

I've been considering a tweak to these string options for a while, and part of what I had in mind involved making Infer case insensitive on method. This is why I said I thought @longouyang's suggestion was a good one, but it wasn't very helpful of me to say that without giving more detail -- sorry about that.

Can we decide on a general strategy for string options

I think we already decided on camel case for these. That's how we ended up with likelyFirst as an option for enumerate for example. I think we use this pretty consistently, for both keys and values.

The change I've been thinking about is the switch from using upper to lower case acronyms. Neither seems entirely problem free when used with camel case, but I think lower case acronyms are probably a bit nicer:

upper lower
MCMC mcmc
incrementalMH incrementalmh
MHKernel mhKernel

I was thinking that having Infer be case insensitive on method would be a way of making this change without breaking too much code. However, it's true that there are other places that are affected, and I'm not sure case insensitivity everywhere is worthwhile.

(Other places affected are the kernel option of MCMC, optimize's estimator option, onlyMAP, maybe others.)

we aren't entirely consistent at the moment. Consider Optimize(model, {optMethod: 'sgd'}); vs Optimize(model, {estimator: 'ELBO'});.

This particular inconsistency arises because optMethod is passed through to adnn which does its own thing. Having things like adagrad and rmsprop be all lower case would fit reasonably well in webppl if we used lower case acronyms as a rule?

What to do?

If we're not sold on case insensitivity everywhere, then I agree we might be best handling the original issue by having a specific error message for that case.

I like the idea of switching to lower case acronyms, but it's not clear it's worth the trouble? If we don't, we can consider fixing the inconsistency of optMethod by an extra mapping from webppl to adnn style.

null-a avatar Sep 17 '16 10:09 null-a

I don't think we want to go case insensitive, because javascript is a case-sensitive language.

I think it's fine to regularize acronyms to lower case, if people want, though I don't think it's crucial.

I think it would be great to have a warning message when a provided option doesn't match any known option. Even better if it gives some "did you mean" options (eg list the closest options in case-insensitive edit distance).

ngoodman avatar Sep 17 '16 14:09 ngoodman