Feature request: Error checking be moved from pypolychord.cpp into a thin python module wrapper.

[appetrosyan]

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.

[williamjameshandley]

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?

[appetrosyan]

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.

[williamjameshandley]

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

[appetrosyan]

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.