Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code tree subsumption #605

Merged
merged 7 commits into from
Sep 19, 2024
Merged

Code tree subsumption #605

merged 7 commits into from
Sep 19, 2024

Conversation

mezpusz
Copy link
Contributor

@mezpusz mezpusz commented Sep 18, 2024

Revival of code tree subsumption. The following fixes were necessary:

  • Flat term size calculation was off for polymorphic equalities.
  • LIT_END was not set properly in the code tree for ILStruct objects.
  • Added equality type uniformly at the start of all equalities in FlatTerm and CodeStack to avoid unsoundness in the two-variable equality case. Some refactors were necessary to do this cleanly.

Debug tests were run, so far no assrtion violations. Correctness checks have been performed with the current SAT subsumption, showing that there is (i) no unsoundness, but (ii) code tree subsumption cannot detect set-based subsumption resolution, only multiset-based.

The final experimental data:

master regression codetree
discount 9981 (47) 9981 (47) 10150 (216)
otter 9720 (42) 9720 (43) 9967 (289)

In parentheses the number of uniques are shown of master to codetree, regression to codetree and codetree to master, respectively.

@easychair
Copy link
Contributor

easychair commented Sep 18, 2024 via email

@quickbeam123
Copy link
Collaborator

This looks awesome. Thank you, @mezpusz .

Even before checking the code change in full, I suggest we make -cts on the default.

Indexing/CodeTree.hpp Outdated Show resolved Hide resolved
@mezpusz
Copy link
Contributor Author

mezpusz commented Sep 18, 2024

Surely it can - we did it ages ago. The only difference is the interpretation of the instruction in charge of setting the literal in the query clause. For multisets, you iterate through previously untried literals. For sets, you iterate through all literals. A

Thanks, it's good to know that it should work. Then perhaps a subsequent PR could fix this, I'm not sure at this point where this should happen in the current code.

@mezpusz
Copy link
Contributor Author

mezpusz commented Sep 18, 2024

This looks awesome. Thank you, @mezpusz .

Even before checking the code change in full, I suggest we make -cts on the default.

I agree, and big thanks to whoever implemented this in the first place. 🙂 Additionally, since the SAT subsumption cross-check on top is relatively cheap after we have found an instance, maybe we could add that as a permanent debug check to make sure no bugs creep in.

@easychair
Copy link
Contributor

easychair commented Sep 18, 2024 via email

@MichaelRawson
Copy link
Contributor

In #265 I removed a dead file called CTFwSubsAndRes which looks a bit like your CodeTreeForwardSubsumptionAndResolution. I think your new shiny code is probably better but might be worth a look to see if you forgot anything.

@mezpusz
Copy link
Contributor Author

mezpusz commented Sep 19, 2024

In #265 I removed a dead file called CTFwSubsAndRes which looks a bit like your CodeTreeForwardSubsumptionAndResolution. I think your new shiny code is probably better but might be worth a look to see if you forgot anything.

Thanks, that's a good suggestion! I have checked the code, fortunately it looks very similar, apart from (i) setting the age of the result for some reason and (ii) doing something with Clause::_auxData. I believe none of these are necessary anymore, but let me know what you think.

@MichaelRawson
Copy link
Contributor

Agreed. Whatever auxData is I'm happy it's dead, but maybe the age needs to be set: @quickbeam123 is the resident expert, I think.

@MichaelRawson MichaelRawson merged commit 38429ed into master Sep 19, 2024
1 check passed
@MichaelRawson MichaelRawson deleted the code-tree-subsumption branch September 19, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants