Improve error messages when necessary optimization library is not loaded

[salbalkus]

Addresses #68 , which arises when the user specifies an optimization library using the optlib parameter that is supported, but they have not currently loaded the library that they have specified.

This PR adds a function that checks if the package specified by optlib has been loaded by the user. It also adds a function to throw an error informing the user that package corresponding to the optlib has not been loaded and to either run using [PackageName] or otherwise specify the correct library. The densratio and densratiofunc functions have been updated to throw the corresponding error in these cases, and unit tests for these cases have been included in runtests.jl.

Additionally, other existing error messages related to the specification of optimization libraries have been made slightly more descriptive and verbose.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.09%. Comparing base (4ffd7a4) to head (b6ae36f).

Files	Patch %	Lines
src/api/estimators.jl	50.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master      #69       +/-   ##
===========================================
+ Coverage   69.65%   83.09%   +13.44%     
===========================================
  Files          18       18               
  Lines         201      207        +6     
===========================================
+ Hits          140      172       +32     
+ Misses         61       35       -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[juliohm]

@salbalkus appreciate your efforts to address various error messages at once with additional code and logic, but as discussed in the issue, we just need a simple patch release with better error messages (strings inside @error calls), which are already thrown. The code added in this PR does a lot more, and relies on a design that is not good for maintenance.

Can you please simplify the PR to only improve the error messages?

[salbalkus]

@juliohm The reason for my contribution is to fix the problem I described in #68 , which will not be addressed by simply changing the @error calls. It seems I have not clearly communicated the issue (partially because I had not totally dug into the entire codebase yet) so let me elaborate.

The @error message calls you referenced are only called if the user attempts to use a DensityRatioEstimator with an optimization library that it hasn't been implemented in. For example, this would occur if the user attempts to call densratio(x, y, KLIEP(); optlib = JuMPLib) because although JuMPLib is a valid option for some estimators, it is not valid for KLIEP.

However, it does not occur if the user calls the DensityRatioEstimator with an optimization library that is implemented but is not yet loaded. For example, if the user calls densratio(x, y, KLIEP(); optlib = Optim) without having first called using Optim, the @error you mentioned will not be reached, because Optim is implemented and Julia will only dispatch on the function with the @error as a fallback when a _densratio function is not already defined for a more specific type of optimization library. As a result of this, the _densratio function will be dispatched on the "correct" version, but an error will be thrown because it is not loaded.

The problem is, as mentioned in #68 , the error that is thrown is MethodError: no method matching iterate(::Nothing) (with no more specific warning from @error) which does not tell the user what they really need to know: that they haven't loaded the correct default library. This is a problem for methods like KLIEP and LSIF where the default libraries are not base Julia. As they often do, if users simply copy and paste any code from the docs other than the very first line, i.e. r = densratio(x_nu, x_de, KLIEP()) or r = densratiofunc(x_nu, x_de, KLIEP()) they will see "no method matching iterate(::Nothing)" which makes it seem like the package doesn't work even in the cases when it is meant to and might drive users away.

I'm definitely open to implementing your feedback and changes on this PR regarding how to design functionality that actually fixes the problem above in a more maintainable way. One improvement, rather than casting the library name into a string and subsetting it (which might break in future releases if the names change), might be to define a dictionary as a global object that maps the object names into package names, i.e. OptimLib => Optim. If you have more specific constraints on the design of the package that you'd like to enforce, please let me know and I'd be happy to work within those bounds!

[juliohm]

This is the error message I get from the MWE in #69 :

It is calling the @error correctly, just the message that needs to be improved. Can you please share the error message you are getting? Why is it not hitting the same code?

[juliohm]

For example, if the user calls densratio(x, y, KLIEP(); optlib = Optim) without having first called using Optim, the @error you mentioned will not be reached, because Optim is implemented and Julia will only dispatch on the function with the @error as a fallback when a _densratio function is not already defined for a more specific type of optimization library. As a result of this, the _densratio function will be dispatched on the "correct" version, but an error will be thrown because it is not loaded.

That seems like a separate issue, which we should address in a separate PR. The fix could be as simple as improving the error message too. Just mentioning that the error may occur when the library is not loaded. No need for additional logic I think, but please let me know if I am missing something.

[salbalkus]

This is the error message I get from the MWE in #69 :

It is calling the @error correctly, just the message that needs to be improved. Can you please share the error message you are getting? Why is it not hitting the same code?

My apologies, @juliohm. It think the MWE from #68 was not showing the "not implemented" error due to an issue with my VSCode setup not properly displaying logs, which I am unable to reproduce; when I run it in the terminal I get the same result as you. That being said, a similar situation actually does occur with KMM; the "not implemented" error message doesn't appear if one runs

using DensityRatioEstimation
densratio([0], [0], KMM())

yielding

ERROR: MethodError: no method matching _kmm_ratios(::Matrix{Float64}, ::Matrix{Float64}, ::KMM{Float64}, ::Type{JuMPLib})

Closest candidates are:
  _kmm_ratios(::Any, ::Any, ::uKMM, ::Type{JuliaLib})
   @ DensityRatioEstimation C:\Users\salba\.julia\packages\DensityRatioEstimation\1Mu92\src\kmm\julia.jl:5

since KMM is missing a "default" function for _kmm_ratios to throw the error.

In this case, is there a reason that @error is used to print an error log instead of throwing an exception using error or throw? From the docs the @error macro is intended for "nonfatal errors", though obviously failing to load the correct optimization package should result in a fatal error. Can I open a new pull request and replace @error "msg" with error("msg") with improved messages? This should also make it easier to test using @test_throws.

[juliohm]

No reason to use @error. Appreciate if you can throw an error properly. Em seg., 8 de jul. de 2024, 12:41, Salvador Balkus ***@***.***> escreveu:

…

This is the error message I get from the MWE in #69 <#69> : [image: image] <https://private-user-images.githubusercontent.com/3345261/346569959-87c2db2b-c4aa-415c-8b7f-ded8ec3e287e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjA0NTA1NDcsIm5iZiI6MTcyMDQ1MDI0NywicGF0aCI6Ii8zMzQ1MjYxLzM0NjU2OTk1OS04N2MyZGIyYi1jNGFhLTQxNWMtOGI3Zi1kZWQ4ZWMzZTI4N2UucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcwOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MDhUMTQ1MDQ3WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YzY4MWQwYmQxMGExODEwOWZlMTUyMGQyOGNhN2VkYjVmZjZmOTQzZjE5YmYxNzRlMWIwMmMzZDA1N2I3YzA4ZSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.0_CZMXZvcQA38LrqooEEXw29eanpj_zAri8D5DX8W6Y> It is calling the @error correctly, just the message that needs to be improved. Can you please share the error message you are getting? Why is it not hitting the same code? My apologies, @juliohm <https://github.com/juliohm>. It think the MWE from #68 <#68> was not showing the "not implemented" error due to an issue with my VSCode setup not properly displaying logs, which I am unable to reproduce; when I run it in the terminal I get the same result as you. That being said, a similar situation actually does occur with KMM; the "not implemented" error message doesn't appear if one runs using DensityRatioEstimation densratio([0], [0], KMM()) yielding ERROR: MethodError: no method matching _kmm_ratios(::Matrix{Float64}, ::Matrix{Float64}, ::KMM{Float64}, ::Type{JuMPLib}) Closest candidates are: _kmm_ratios(::Any, ::Any, ::uKMM, ::Type{JuliaLib}) @ DensityRatioEstimation C:\Users\salba\.julia\packages\DensityRatioEstimation\1Mu92\src\kmm\julia.jl:5 since KMM is missing a "default" function for _kmm_ratios to throw the error. In this case, is there a reason that @error is used to print an error *log* instead of throwing an exception using error or throw? From the docs ***@***.***> the @error macro is intended for "nonfatal errors", though obviously failing to load the correct optimization package should result in a fatal error. Can I open a new pull request and replace @error "msg" with error("msg") with improved messages? This should also make it easier to test using @test_throws. — Reply to this email directly, view it on GitHub <#69 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3KYUVJBJPWHJ5SP3E3ZLKXKBAVCNFSM6AAAAABKP3YJIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJUGQ3TIMJZGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[juliohm]

@salbalkus did you have a chance to take a look into this? We just need to throw and actual error with a more informative error message.

[salbalkus]

@salbalkus did you have a chance to take a look into this? We just need to throw and actual error with a more informative error message.

Yes, I've written more informative error messages. Currently I'm setting up unit tests to make sure the code is properly covered and they're being thrown in the right situation. I can open a pull request tonight.

[salbalkus]

This new commit reverts my previous change to check if the necessary libraries are loaded and adds more maintainable error throwing.

• I've replaced the previous code with a few basic functions in utils.jl to construct informative error message strings and throw errors from every default function that had previously been adorned with @error. Let me know if these functions belong in a different .jl file.
• I've also replaced the unit tests to check that the necessary errors are thrown. Currently they are using the generic ErrorException; happy to define a custom type if you feel it is needed.
• Also, a few errors were not being reached properly; I made some small changes to the parameter typing in the "default" functions to ensure intended behavior.

[juliohm]

That is really good! Thanks for the awesome contribution!

Merging and releasing a patch 👍🏽

[salbalkus]

Thank you @juliohm - happy to help!