search

probcomp / hierarchical-irm

Probabilistic structure discovery for rich relational systems
Apache License 2.0
1 stars 1 forks source link

Make small fixes to get tests running #2

Closed alex-lew closed 1 month ago

alex-lew commented 1 month ago

This PR implements several small changes that I had to make in order to get make tests to run from start to finish:

Presumably these tests used to run, so maybe some of these fixes are necessary only due to quirks on my machine? Curious to know if (1) the tests run for others without these changes; (2) whether these changes break anything for others.

ThomasColthurst commented 1 month ago

For what it is worth, I was able to successfully run tests on a Debian Linux machine without these changes.

alex-lew commented 1 month ago

@ThomasColthurst Thanks -- that's good to know. Did you get a chance to check if they still run on Debian Linux with these changes?

The first change (virtual destructor) I can imagine would have platform- or compiler-specific repercussions, but it's interesting that the other changes aren't necessary on Linux. I wonder what could be causing this... maybe different PRNG?

The particular tests that failed for me were L75-76 in test_irm_two_relations.cc:

        assert(abs(Z) < 1e-10);
        assert(abs(exp(p0) - expected_p0[x1].at(x2)) < .1);

The first line failed by a small amount (abs(Z) was small but not small enough) for indices (0, 100) in the for loop, and was fixed by changing 1 + domain->crp.N to domain->crp.alpha + crp.N. The second line failed for indices (10, 0), and was fixed by changing loop index to 20.

I've pinged Feras and asked him to weigh in, too.

Joaoloula commented 1 month ago

@alex-lew: I reproduced you and @ThomasColthurst's results:

so LGTM, maybe just change the PR name to "Make small fixes to get tests running on mac" for documentation purposes?

alex-lew commented 1 month ago

@Joaoloula Thanks!!

I think the warnings when compiling are due to the first change here, making the destructor of Distribution virtual. And I think it makes sense that this would be platform-dependent.

The thing that I'm confused about is why you don't get errors on Debian without the other two fixes. So I'd love to confirm that the other two fixes are really correct, at a semantic level. But maybe safe to merge for now, if we think they are?

fsaad commented 1 month ago

Thanks for all these fixes.

  • The Distribution base class's destructor is now virtual

Agreed, base classes' destructors should be virtual; otherwise behavior is ill-defined.

In the logp methods of Relation and IRM, there was a tacit assumption that domain->crp.alpha was equal to 1; now we use the actual value in domain->crp.alpha for each domain.

Agreed. The same (invalid) assumption also appears in the Python code:

The two-relation-IRM test file referred to item 10 in the data file using the integer 10, but should have used the integer 20, since "10" is introduced after 20 other items.

I do not remember the exact details, but the fix looks correct. Let us make the code more robust by extracting the codes corresponding to the items as follows:

    auto item_to_code = std::get<0>(encoding);
    auto code_item_0 = item_to_code.at("D1").at("0");
    auto code_item_10 = item_to_code.at("D1").at("10");
    auto code_item_novel = 100;

    map<int, map<int, double>> expected_p0 {
        ... code_item_0, code_item_10, code_item_novel ...
    };
alex-lew commented 1 month ago

Great, thanks so much! I've pushed a fix to the Python version, and added the more robust lookup as you suggested. I think the codes are different for D1 and D2, so I added a couple more variables to perform the lookups separately:

https://github.com/probcomp/hierarchical-irm/blob/a77ffdbd40781014b7d64d159189129121967df4/cxx/tests/test_irm_two_relations.cc#L59-L64

I think this should now be good to merge. Thanks all!