Open appetrosyan opened 4 years ago
I think that it's best to leave the cpp error checking for now, as in theory there may be other non-python interfaces which go via the cpp which do rely on these checks. If the python checking interface in #50 works properly, will it matter that these are still lurking under the hood?
The issue here are the checks in _pypolychord.cpp
, which are not part of the C++ api altogether. If you're using an internal function, having no error checking is the least of your worries.
The issue here is that the error checking in C++ is not idiomatically Python
-ic, and it's duplicating the functionality that should be included in the settings
object to begin with.
The former is a problem because of non-existent tracebacks. You don't really know what went wrong, so if you can catch the errors before they're passed to C++, a python programmer would know what to do.
The latter is an issue of code duplication. If someone later down the line introduces a feature that allows non-double arrays or slightly changes the run_polychord
's signature they might need to duplicate the changes in C++ too. A transdimensional sampler is one such case.
The last point is that Pythonic error checking in C++ is not maintainable. I can barely keep track of the refcounts, and It's quite easy to make a mistake. If the Python interface handles the checking, the C++ code can focus on adapting the python objects to the C++ interface. The way it is now, the responsibility of _pypolychord.cpp
is a little too broad.
I think I see the issue now. I imagine that most of the error checking can safely go if the python wrapper is robust enough. Some of it however is important for raising exceptions appropriately from the loglikelihood/prior/dumper functions, but you should feel free to submit a pull request deleting what you feel is unnecessary, and we can discuss further there
Some of it however is important for raising exceptions appropriately from the loglikelihood/prior/dumper functions.
While writing the project up, I implemented a couple of sanity checks in this file. The idea is to check the prior for some point in the hypercube, and fork off a thread to test if the likelihood fed with the result raises an exception. This let catch the wrong nDims
more than once.
The error checking in
pypolychord.cpp
is cumbersome. If #48 is fixed, we may no longer need to check the congruency of the settings. Otherwise, it may still be easier to work with idiomatic python and only use the C++ module for running PyPolyChord.