search

DataverseLabs / pyinterpolate

Kriging | Poisson Kriging | Variogram Analysis
https://pyinterpolate.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
151 stars 26 forks source link

[BUG] Add "safe" method to the error message when `TheoreticalVariogram.autofit()` is performed with an invalid model type #395

Closed SimonMolinsky closed 1 year ago

SimonMolinsky commented 1 year ago

Package version 0.4.2

Describe the bug If I pass a wrong model_types parameter to TheoreticalVariogram.autofit() I get this error message:


KeyError: "Defined model name basic not available. You may choose one from ['circular', 'cubic', 'exponential', 'gaussian', 'linear', 'power', 'spherical'] instead."

It should include "all" and "safe" models too.

Input Data Schema Any string value not supported by model_types parameter.

Expected behavior


KeyError: "Defined model name basic not available. You may choose one from ['all', 'safe', 'circular', 'cubic', 'exponential', 'gaussian', 'linear', 'power', 'spherical'] instead."

Screenshots n/a

Desktop (please complete the following information):

Additional context n/a

Harshal662 commented 1 year ago

in which file that code is present?

SimonMolinsky commented 1 year ago

It is in this function: https://github.com/DataverseLabs/pyinterpolate/blob/76ea448072a5b751e02499b09da4f29c209b5492/pyinterpolate/variogram/theoretical/semivariogram.py#L885

However, it checks model names for fit and autofit, but autofit has two additional options "safe" and "all". So maybe the new message should state:

KeyError: "Defined model name basic not available. You may choose one from ['circular', 'cubic', 'exponential', 'gaussian', 'linear', 'power', 'spherical'] instead, or use additional 'all' or 'safe' parameters if you performs autofit()."

Harshal662 commented 1 year ago

https://github.com/DataverseLabs/pyinterpolate/blob/76ea448072a5b751e02499b09da4f29c209b5492/pyinterpolate/variogram/theoretical/semivariogram.py#LL173C1-L182C36

in this line it is checking of the models given all and safe models are not their, it is not checking for them

SimonMolinsky commented 1 year ago

Thanks! 🙏 I know about that but don't have time to code it myself. That's why I've posted the issue :) It is relatively easy to make this correction, and even a person that is not "fluent" in the package can work with it. I was hoping (and still hope) that someone else would grab it. I work now on a CLI of the package, so I don't have time for every enhancement/correction :)

Usually, I work with issues like that just before the new minor release, so if no one is interested, I will get to it in the future.