fix: remove type checking in Predicate.__call__ #127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit removes a check on the types of the new terms passed as arguments to Predicate.__call__. The reason is that the check is not "complete": for example, the check returns false when a new term whose types are a subset of the types of the term being replaced, while the correct result is true. More fundamentally, the problem is that the class Predicate, in its current state, does not have information on the type hierarchy of the domain the predicate belongs to. Having this check here adds more problems than benefits. The final type check will be performed when instantiating the Domain class while leaving the flexibility to the user to instantiate temporary predicate objects, even with terms whose type is incompatible with the target type hierarchy of the PDDL domain. Therefore, rather than having a half-working feature, it is better to remove it altogether. The idea is still relevant, but it may require a distinction between 'PredicateDefinition' (which has a pointer to the domain or the type hierarchy) and 'Predicate' (the actual instance of the predicate definition, with only constants).
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
- Coverage 88.12% 88.12% -0.01%
==========================================
Files 24 24
Lines 1752 1751 -1
Branches 233 232 -1
==========================================
- Hits 1544 1543 -1
Misses 148 148
Partials 60 60
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This commit removes a check on the types of the new terms passed as arguments to Predicate.call. The reason is that the check is not "complete": for example, the check returns false when a new term whose types are a subset of the types of the term being replaced, while the correct result is true.
More fundamentally, the problem is that the class Predicate, in its current state, does not have information on the type hierarchy of the domain the predicate belongs to. Having this check here adds more problems than benefits. The final type check will be performed when instantiating the Domain class while leaving the flexibility to the user to instantiate temporary predicate objects, even with terms whose type is incompatible with the target type hierarchy of the PDDL domain.
Therefore, rather than having a half-working feature, it is better to remove it altogether. The idea is still relevant, but it may require a distinction between 'PredicateDefinition' (which has a pointer to the domain or the type hierarchy) and 'Predicate' (the actual instance of the predicate definition, with only constants).
Fixes
Partially addresses issues in #98
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply.Further comments
Given that there are different overlapping issues regarding validation, I propose to address them in a separate refactoring as attempted in #84 (relatively soon).