search

singnet / pln

Probabilistic Logic Network (PLN) implemented on top of the Unified Rule Engine (URE). https://wiki.opencog.org/w/Probabilistic_logic_networks
Other
1 stars 4 forks source link

Temopral Reasoning #35

Closed tanksha closed 3 years ago

tanksha commented 3 years ago

This PR Includes

tanksha commented 3 years ago

@ngeiswei I have compiled the recent works in these PR, please review them when you have time. Thanks

ngeiswei commented 3 years ago

Thanks @tanksha ! Very minor first remark: PLNRulesIntegrationTest should be renamed PLNRulesIntegrationUTest.

ngeiswei commented 3 years ago

Oh, apologies, you didn't write PLNRulesIntegrationTest!

ngeiswei commented 3 years ago

Also, it is generally better if you create a new branch as opposed to using master. It makes it easier to organize your work and clearer for others as well.

ngeiswei commented 3 years ago

Also, when possible, before creating the PR, it's better to rebase your branch onto the upstream/master, as opposed to say merge the upstream/master into your branch after creating the PR, that way your commits appear at the top of the git history, which is slightly easier to review.

ngeiswei commented 3 years ago

CircleCI fails due to not having pln being installed before running the unit tests. Joel Pitt developed a way to avoid that. I recall having being able to use it but I also recall that it's tricky. So you should either fix pln so that it doesn't need to be installed to run its unit tests, or you should fix circleci to install pln before running the unit tests.

For fixing circleci you can have a look at asmoses, which does install asmoses before running its unit tests. And for fixing pln you can have a look at the atomspace repo. Maybe we can look into that during the call.

ngeiswei commented 3 years ago

@tanksha, since TemporalReasoningUTest is only testing rules individually I think it would be more appropriate to have it under the rules subfolder alongside PLNRulesUTest.cxxtest, and to rename it to TemporalReasoningRulesUTest.

ngeiswei commented 3 years ago

BTW, when you'll make integrated unit tests you'll have to use the PLN's API from the pln guile module. I'm gonna fix PLNRulesIntegrationTest to use the pln module which should give you an example. I'll try to fix the need for installing pln to run the utests as well, but I don't promise anything, as I said it's tricky.