IDSIA / crema

Crema: Credal Models Algorithms
https://crema-toolbox.readthedocs.io/
GNU Lesser General Public License v3.0
10 stars 4 forks source link

Crepo compatibility with crema 0.2.0 #80

Closed rcabanasdepaz closed 3 years ago

rcabanasdepaz commented 3 years ago

I have done a first review of the crepo java code for making it compatible with crema 0.2.0 (https://github.com/IDSIA/crepo/tree/dev-crema-0.2.0). Here I detail some points that I do not know how to do in the new version of crema or that I would like to discuss:

1) A random v-factor can be created with the following method, but how do you limit the number of decimals?

    VertexFactorUtilities.random(leftDomain, Strides.empty(), nVert); 

2) In a existing H-factor, how can I specify set of constraints for a given parent combination from a linearContraintSet? This was done with setLinearProblemAt. This method is not available anymore.

    public static SeparateHalfspaceFactor vertexToHspace(VertexFactor factor) throws IOException, InterruptedException {

        SeparateHalfspaceFactor HF = SeparateHalfspaceFactorFactory.factory()
                .domain(factor.getDataDomain(), factor.getSeparatingDomain()).get();

        for(int i=0; i<factor.getSeparatingDomain().getCombinations(); i++) {
            VertexFactor VFi = new VertexDefaultFactor(factor.getDataDomain(), Strides.empty(), new double[][][]{factor.getData()[i]});
            SeparateHalfspaceDefaultFactor HFi = (SeparateHalfspaceDefaultFactor) Convert.margVertexToHspace(VFi);
            HF.setLinearProblemAt(i, HFi.getLinearProblemAt(0));
        }

        return HF;

    }

3) Make public the method SeparateHalfspaceDefaultFactor::setLinearProblemAt.

cbonesana commented 3 years ago
  1. I don't understand the question. Why do you need to limit the number of decimals? There is no guarantee that a decimal number can be represented in a floating point double precision number. If you want such precision we need to work with completely different number format (and slower mathematical operations).

  2. All Factors are now immutable. If you want to build a new Factor you need to use the relative Factory class. In your case you want to build a VertexFactor then you need to use a VertexFactorFactory. There is an example on how to use these class.

  3. See 2.

rcabanasdepaz commented 3 years ago

About 2 and 3, ok it make sense to have them inmutable. I will have a look to it and eventually I might need to extend the functionality of the Factories.

About 1, clearly we cannot change the number of decimals in a float number. What I am saying is to work with rounded probabilities. For instance, instead of working with random distributions such that [0.852123453, 0.14787654699999997], we might prefer to have [0.85, 0.15]. The linear programming solver tend to have less problems in this case (even I know that there might be still some little precision error as 0.85 could be stored as 0.8500000000001).

A possible solution would be to have rounds the distributions. Do you know if this operation is already implemented or where could I add it?

davidhuber commented 3 years ago

I don't think we currently have any such rounding implemented.

cbonesana commented 3 years ago

I'm a little bit sceptical: it just looks like you want a formatter when you print the number since behind the scene, as you said, there is no guarantee that a "nice" number is stored in a "nice" way.

Maybe it is just a matter of have a special nice-random-number generator instead of a whole setup for factor generators.

@davidhuber this sounds like the idea of having probability distributions as factors.

rcabanasdepaz commented 3 years ago

So what we can do is to add some functions for formatting and cleaning the distributions. These might be added in the clases with the utilities for factors, so we could keep the rest of the code clean. As I have lot of code for this also in credici, I can take care of this.

cbonesana commented 3 years ago

Class FactorUtil contains utility methods for the algebra of the factors. Other FactorXUtilities classes contains all the methods that should not be in the implementation classes. The majority of these methods, when not all, are static.

I think that you just need a better way to control the random generation of values. We can update the RandomUtil class with new methods and then add a .random() method to the various FactorXFactory classes. Since factors are unmutable, this is the place where the stored data can be changed.

...or we implement new Factor implementation for the same interface. This was the original way to convert Factors to interfaces so we can have multiple implementations with different behavior :)

rcabanasdepaz commented 3 years ago

Crepo is already working with crema 0.2.0. I close the issue