[BUG] Constructor function uses setParameter instead of class constructors

[bjoelle]

I encountered this while tracking down why a = Probability(-1) works when the checks in the class constructors should block that
basically ConstructorFunction::execute in ConstructorFunction.cpp builds the class from a template and then calls setParameter or setConstParameter, and Probability::setConstParameter has no checks on the value passed to it

we could solve the specific issue by adding checks to Probability::setConstParameter but this seems like a problem that should have a more generic solution

[davidcerny]

This is the source of issue #348.

[bredelings]

Perhaps a more generic solution would modify how a ConstructorFunction works? Or, even more generally, we could express some types (like Probability) in terms of a base type and a constraint.
So,

• RealPos = (Real, x >=0)
• Probability = (RealPos, x>=0 and x <= 1)
  Right now these constrained types don't really seem to enforce constraints, they mostly serve as a note that the constraint exists for the purpose of finding argument-type mismatches.

Looking at the Probability class in src/revlanguage/datatypes/basic/Probability.cpp, it looks like the value can be set in 3 ways:

• Probability::Probability( double x )
• Probability::Probability( RevBayesCore::TypedDagNode *x )
• void Probability::setConstParameter(const std::string& name, const RevPtr &var)

Only the first way checks the value. We could certainly add a check to setConstParamter( ). I wonder if we could also add a check to Probability::Probability( RevBayesCore::TypedDagNode<double> *x ).

[davidcerny]

Any further thoughts on this? My take would be that for now, an inelegant fix is probably better than keeping the error in until we get around to restructuring the whole type system (which unfortunately really is a mess; see also issue #308). Happy to open a quick PR for this if that's what we agree to do.

[bredelings]

Yeah, in practice would should probably fix it new, even before we have a wider solution. You may as well prepare a PR.

@bjoelle Do you have any ideas about a "more generic solution"? Perhaps one that is less invasive than the one I mentioned?

[bjoelle]

so my ideas on a more generic solution would be

• as you mentioned, switch the ConstructorFunction to use a constructor ? pro is that it would apply to every class without added effort, con is I'm not sure how well that would work because I'm not sure why things were setup in that way in the first place
• have a "validate" function or similar in all rb objects which would get called by both the constructor and the setParameter ? pro is it doesn't change anything in the current structure aside from limiting code duplication, con is it still requires the developer to pay attention to setup properly for new objects