bbopt / nomad

NOMAD - A blackbox optimization software
https://nomad-4-user-guide.readthedocs.io/
GNU Lesser General Public License v3.0
110 stars 24 forks source link

Inconsistent return value from solveNomadProblem #164

Open paulapatience opened 5 months ago

paulapatience commented 5 months ago

solveNomadProblem's return type is bool, but inconsistently returns 1 on invalid arguments (i.e., boolean true), returns 0 on presumable success (i.e., boolean false), and finally returns true when catching an exception.

It would appear that solveNomadProblem indicates success with a return value of 0 (false), and failure with a return value of 1 (true). Unfortunately, this conflicts with the style followed by the addNomad*Param functions which return true on success, which is indeed the usual convention for functions indicating success or failure by a boolean return value.

salomonl commented 5 months ago

@paulapatience It could be corrected, but follows a c convention (the exit(1) or exit(0)). At long term, the goal is to return an int flag, indicating the state of the resolution, but it is not yet the case.

salomonl commented 5 months ago

Corrected for the next release.

paulapatience commented 5 months ago

The C convention for indicating error or success via an integer return value is usually negative integers on error, 0 on success.

I think the flag indicating the state of the resolution would fit better as part of the NomadProblemInfo struct. It could be a field of the struct, and a function like getNomadProblemState would return its value. Unfortunately, the name NomadProblemInfo suggests a static description of a problem, which a state flag most definitely is not. Too bad the flag could not go into a hypothetical NomadProblem struct. This would require dropping the typedef for NomadProblemInfo* in favor of explicitly indicating pointers, which serves as a reminder that they may need to be freed — indeed I have not seen many such pointer typedefs in C++, though I have seen them in C. I do not know what the NOMAD project's stance is on breaking backwards compatibility.