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

Transparent pddl #62

Merged
merged 5 commits into from
Jan 2, 2021
Merged

Transparent pddl #62

merged 5 commits into from
Jan 2, 2021

Conversation

fmrico
Copy link
Contributor

@fmrico fmrico commented Dec 30, 2020

Dear all,

This is a big change, and I would like to have feedback from you @teyssieuman @mark-sentaca @jjzapf @rossj13

Issues #61 #35 #21, at least, are motivated by the limitation of how plansys2_domain_expert stores the pddl domain. That's because I wanted to manage (merge) several domains. This can be useful in an application like this .

This PR contains an alternative way to solve this. With these changes, the model is not deep parsed, with the limitation that you found. Now, the domains are merged, respecting the original pddl content.

I hope you find it interesting. Please, try this branch to see if it solves the limitation that you found.

Best

Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>
Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>
@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #62 (61f6941) into master (bd7708d) will increase coverage by 0.01%.
The diff coverage is 30.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   23.07%   23.08%   +0.01%     
==========================================
  Files         116      119       +3     
  Lines        6137     6454     +317     
  Branches     3562     3811     +249     
==========================================
+ Hits         1416     1490      +74     
- Misses       2231     2279      +48     
- Partials     2490     2685     +195     
Flag Coverage Δ
unittests 23.08% <30.60%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rt/src/plansys2_domain_expert/DomainExpertNode.cpp 15.71% <ø> (+1.14%) ⬆️
...2_problem_expert/test/unit/problem_expert_test.cpp 17.35% <ø> (ø)
...ys2_domain_expert/test/unit/domain_expert_test.cpp 8.83% <11.11%> (+0.35%) ⬆️
...ys2_domain_expert/test/unit/domain_reader_test.cpp 16.12% <16.12%> (ø)
...omain_expert/test/unit/domain_expert_node_test.cpp 18.42% <18.42%> (ø)
...expert/src/plansys2_domain_expert/DomainExpert.cpp 48.44% <44.89%> (+0.62%) ⬆️
...expert/src/plansys2_domain_expert/DomainReader.cpp 50.42% <50.42%> (ø)
...s2_pddl_parser/src/plansys2_pddl_parser/Lifted.cpp 9.09% <0.00%> (-45.46%) ⬇️
...pddl_parser/src/plansys2_pddl_parser/ParamCond.cpp 0.00% <0.00%> (-44.45%) ⬇️
..._pddl_parser/src/plansys2_pddl_parser/Function.cpp 25.00% <0.00%> (-16.67%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd7708d...61f6941. Read the comment docs.

@mark-sentaca
Copy link

This looks very interesting. We will be taking a closer look. Thanks!

@mark-sentaca
Copy link

We've worked through the build changes (which turned out to be pretty minor) but are still seeing the 'assign' token not known error. Are there any changes in terms of usage we need to change in order to ensure our test application is using the new functionality? Thanks

@fmrico
Copy link
Contributor Author

fmrico commented Dec 31, 2020

We've worked through the build changes (which turned out to be pretty minor) but are still seeing the 'assign' token not known error.

@mark-sentaca, in the first tutorial, I describe how to run directly popf in the terminal with a domain and a problem file.

ros2 run popf popf /tmp/domain.pddl /tmp/problem.pddl

Could you please try if this token is accepted? If not, try the same with TFD?

@mark-sentaca
Copy link

Thank you - that has exposed a few errors in our pddl definition that I will have to debug. Interestingly, the token appears to be accepted because the errors are not related to the 'assign' keyword.

Will keep you in the loop as we debug this. We are relatively new to pddl so that's been a learning curve with the team here.

@mark-sentaca
Copy link

The error we were seeing from PlanSys2 was:
[plansys2_node-1] [ERROR] [1609429229.751065720] []: Caught exception in callback for transition 10
[plansys2_node-1] [ERROR] [1609429229.751072881] []: Original error: 'assign' does not name a known token
[plansys2_node-1] [WARN] [1609429229.751085925] []: Error occurred while doing error handling.
[plansys2_node-1] [ERROR] [1609429229.751090880] [domain_expert]: [domain_expert] Error transition
[plansys2_node-1] [WARN] [1609429229.751474353] [domain_expert_lc_mngr]: Failed to trigger transition 1

fmrico added 3 commits January 2, 2021 17:21
Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>
Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>
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.

2 participants