search

dfm / george

Fast and flexible Gaussian Process regression in Python
http://george.readthedocs.io
MIT License
445 stars 128 forks source link

"metric" is linear but "metric_bounds" is log #150

Open arodland opened 2 years ago

arodland commented 2 years ago

If you do ExpSquaredKernel(metric=30, metric_bounds=[(10, 100)]) it will fail with an unhelpful "non-finite log prior value". Why? Because the initial value is 30, but the bounds are e^10 and e^100, which doesn't include the initial value. Same applies if you do Metric(30, bounds=[(10, 100)], ndim=1). You really wanted to write ExpSquaredKernel(metric=30, metric_bounds=[(np.log(10), np.log(100))]).

It would be nicer to change the interface (either to take the bounds as linear and take the log internally, or rename the arg as log_metric_bounds) but that would be a breaking change, so perhaps you could just clearly document the existing behavior and provide an example?

Also, the code that produces that "non-finite log prior" message knows that it's doing a bounds check, so how about making the message something more like metric prior value 30 outside of bounds [22026.46579, 2.68811714e+43] which would tip people off as to where things went wrong.

dfm commented 2 years ago

Yeah - that interface is all a little janky! All of these suggestions are good. I don't have a huge amount of time for george maintenance these days, but I'd be very happy to review a PR.

arodland commented 2 years ago

@dfm if you let me know which approach is your favorite I'll give it a go.

dfm commented 2 years ago

Awesome!! I think the easiest first step would be the improved error message. What do you think?