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

Added option for removing the object types when writing to file #144

Open
wants to merge 5 commits into
base: dev-0.9.0
Choose a base branch
from

Conversation

Martin36
Copy link

Fixes issue #142

Also provided an example notebook of how to use the new feature and how to write to a PDDL file

@miquelramirez miquelramirez self-requested a review October 24, 2023 22:07
@miquelramirez miquelramirez self-assigned this Oct 24, 2023
@miquelramirez miquelramirez added the contribution Contributions to Tarski label Oct 24, 2023
@miquelramirez miquelramirez changed the base branch from master to dev-0.9.0 October 24, 2023 22:19
Copy link
Member

@miquelramirez miquelramirez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Martin36,

thank you for the PR. I have changed the target branch of your PR from aig-upf:master to aig-upf:dev-0.9.0, which I have created to stage changes for the next release.

I have no objection to the implementation. Before I can accept this PR though, it would be necessary to amend the PR adding the following:

  • Check that no sorts have been defined for the FirstOrderLanguage instance being used as the domain theory. Otherwise, potentially important information would be lost.
  • If there are sorts defined, raise an exception to alert the user that sort information is going to be lost.

Not necessary but more of a suggestion is the following feature:

  • External to the writer, it could be useful to add a new submodule to tarski.transform that transforms a FirstOrderLanguage instance with sorts defined that are subsets of Object into a semantically equivalent FirstOrderLanguage instance where for each user-defined sort, there is a unary predicate for it. For instance, if we have the sort $Foo$ in instance A, then we have the predicate $Foo \subset Object$ in instance B.

@Martin36
Copy link
Author

Hi @Martin36,

thank you for the PR. I have changed the target branch of your PR from aig-upf:master to aig-upf:dev-0.9.0, which I have created to stage changes for the next release.

I have no objection to the implementation. Before I can accept this PR though, it would be necessary to amend the PR adding the following:

  • Check that no sorts have been defined for the FirstOrderLanguage instance being used as the domain theory. Otherwise, potentially important information would be lost.
  • If there are sorts defined, raise an exception to alert the user that sort information is going to be lost.

Not necessary but more of a suggestion is the following feature:

  • External to the writer, it could be useful to add a new submodule to tarski.transform that transforms a FirstOrderLanguage instance with sorts defined that are subsets of Object into a semantically equivalent FirstOrderLanguage instance where for each user-defined sort, there is a unary predicate for it. For instance, if we have the sort Foo in instance A, then we have the predicate Foo⊂Object in instance B.

Done

@Martin36
Copy link
Author

Martin36 commented Feb 15, 2024

@miquelramirez Is there still something missing from the PR, or is it ready to be merged?

@miquelramirez
Copy link
Member

Apologies @Martin36 I somehow missed the notification for your response to my last message. Will look into the latest amendments later today.

@miquelramirez
Copy link
Member

Apologies @Martin36 I somehow missed the notification for your response to my last message. Will look into the latest amendments later today.

Actually, checking this on my desktop and not my mobile phone and I see that I did not miss any messages. There are review messages are pending. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Contributions to Tarski
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants