Create a non-prefix rddl parser.

Issue #26 resolved
tkeller
repo owner created an issue

A parser that does not need the intermediate step of translating rddl to rddl prefix would be nice. (issue from 28.06.2016)

Comments (16)

  1. tkeller reporter

    Dorde started to work on this issue (on a bitbucket branch, if you are interested in seeing the current version let me know and I'll share the repo), and I had some comments on the code:

    • includes in header only if required. otherwise, include in .cc
    • replace tabs with spaces
    • is helper_methods necessary, or can it be moved to parser.ypp?
    • merge rddl with planning_task?
    • use {} around if-body even if it's just a one-liner
    • do not declare multiple variables in one line (e.g.CaseDefine in rddl.h)
    • use const whenever possible (e.g., getter methods)
    • clean up pointers where possible
    • do we need the maps in parser.ypp -> keep it and include it in the workflow presentation of flex/bison
    • addPVar in DomainList in rddl.h should be setPVars
  2. Đorđe Relić

    Progress update:

    Done:

    • replace tabs with spaces
    • is helper_methods necessary, or can it be moved to parser.ypp?
    • use {} around if-body even if it's just a one-liner
    • do we need the maps in parser.ypp -> keep it and include it in the workflow presentation of flex/bison
    • addPVar in DomainList in rddl.h should be setPVars
    • merge rddl with planning_task?

    Questions:

    • use const whenever possible (e.g., getter methods) (analyse other possible const fields or functions)
    • includes in header only if required. otherwise, include in .cc (already done?)
    • do not declare multiple variables in one line (e.g.CaseDefine in rddl.h) (already done?)

    TODO after consultation:

    • clean up pointers where possible
  3. tkeller reporter

    @Đorđe Relić and I just had a meeting and discussed what else there is to do. Here is the list we came up with:

    • in parser.ypp, change the nullptr that is created when the requirements section is empty in the same way as it has been done in rev 235 for state action constraints
    • clean up unnecessary comments and move the remaining ones to a separate line if the char limit is violated
    • find better names for member variables (e.g., lConstList)
    • get rid of external maps and helper functions and introduce a single external PlanningTask object instead
    • the type and object system exists twice; redundancy can be removed if point above is implemented; also related to this is the TODO in line 557 of parser.ypp
    • move non-member requirements stuff to PlanningTask class
    • is PvariablesInstanceDefine class necessary or can it be combined with another class? (there is no need to have different classes if they are the same concept but occur once in NonFluents and once in the Domain section)
    • rename Pvar -> Variable and Cpf -> CPF
    • Schematic or Lifted instead of Define / Definition
    • Merge Domain and DomainList?
    • Remove CpfDefinition destructor
    • replace addReward with setRewardCPF (and replace "if(!domain->getReward())..." with "assert(!domain->getReward())"

    Changes that are postponed to be handled in other issues:

    • replace strings with objects, e.g., Parameter instead of std::string (includes changes in LogicalExpression::instantiate, though)
    • currently, parsing is not timed (timing is only in RDDLTask::execute, which is called after parsing and basically contains the "old" main).
    • Makefile doesn't realize if only parser.ypp is changed; probably makes sense to wait with this until we have discussed if we want to switch to another build system like cmake or not
  4. Đorđe Relić
    • is PvariablesInstanceDefine class necessary or can it be combined with another class? (there is no need to have different classes if they are the same concept but occur once in NonFluents and once in the Domain section) -> Definition of Variables in Instance and in NonFluetns is different according to Scott's grammar rules. Merging is possible but before any work done, is there any reason for those two to be different in definition? Note: PvariableInstanceDefine is now renamed to VariableInstanceSchematic.
    • Merge Domain and DomainList? -> Domain and DomainList are separate because of the requirements section. Requirements section can be defined only once in the beginning, while other sections can be defined several times and in whichever order. These rules are defined like this by Scott so I've implemented them like this. In case, requirements section can be defined multiple times and defined wherever in the domain file, those two classes can be merged.

    Other bullet requests from upper group have been addressed and the code is online plus the bugfix for Makefile not recognizing the change of parser.ypp. In my opinion, Make file could be improved a bit more, concerning the lines of code concerned for running bison and flex.

    Working on these requests, I realized that there is some possibly redundant code so I'd like to change/delete which I will address in the next few days. The main reason for this redundancy is the global variable RDDLTask that can be used all over parser.ypp.

  5. tkeller reporter

    Merge Domain and DomainList? -> Domain and DomainList are separate because of the requirements section. Requirements section can be defined only once in the beginning, while other sections can be defined several times and in whichever order. These rules are defined like this by Scott so I've implemented them like this. In case, requirements section can be defined multiple times and defined wherever in the domain file, those two classes can be merged.

    I am nor sure if I understand this correctly. Which section may exist more than once? There may be only one types, reward, cpfs, state-action-constraints etc section in a domain definition, and there may be only one domain, objects and non-fluents section in the non-fluents definition and the same is true for the possible entries of an instance definition. If Scott's code allows for multiple entries, we can ask him if that is on purpose, but I don't see the advantage.

    If all sections are only allowed once (within the domain definition), can you also merge Domain and DomainList?

  6. tkeller reporter

    is PvariablesInstanceDefine class necessary or can it be combined with another class? (there is no need to have different classes if they are the same concept but occur once in NonFluents and once in the Domain section) -> Definition of Variables in Instance and in NonFluetns is different according to Scott's grammar rules. Merging is possible but before any work done, is there any reason for those two to be different in definition? Note: PvariableInstanceDefine is now renamed to VariableInstanceSchematic.

    How are they different? Of course, only static variables are initialized in the non-fluents definition and ones that could be dynamic in the init-state section of the instance definition, but I don't think we care about that difference during parsing (and we check if variables are static or not automatically).

  7. Đorđe Relić

    I am nor sure if I understand this correctly. Which section may exist more than once? There may be only one types, reward, cpfs, state-action-constraints etc section in a domain definition, and there may be only one domain, objects and non-fluents section in the non-fluents >definition and the same is true for the possible entries of an instance definition. If Scott's code allows for multiple entries, we can ask him if that is on purpose, but I don't see the advantage.

    If all sections are only allowed once (within the domain definition), can you also merge Domain and DomainList?

    According to Scott's design of RDDL grammar, all sections can be defined multiple times and in whichever order except for the requirements section. The requirements section was added separately (as optional) and it has to be defined before every other section (hence the two classes), but also can be defined multiple times. If we make a change in grammar design so that requirements section has the same rules as other sections (meaning that it can be defined in whichever order), we can merge Domain and DomainList.

    "defined multiple times" holds as far as the design of grammar goes. Maybe Scott's implementation of corresponding classes disables multiple definition of sections, which would be a way for us to solve that issue as well. Example of "defined multiple times" is below:

    types {
            elevator : object;
            floor    : object;
        }; 
        pvariables { 
        ......
        };
    
    types {
            escalator : object;
            room    : object;
        }; 
    
  8. Đorđe Relić

    @tkeller I don't see any advantage either, but that is how it is implemented. This is currently supported in both Scott's parser and in non-prefix for PROST. Possible solutions are to keep track of the sections which have been defined in RDDLTask and throw error when user tries to define a section again or to define an exact order of the definition of sections. However, it can lead to incompatibility with Scott's parser.

  9. Đorđe Relić

    How are they different? Of course, only static variables are initialized in the non-fluents definition and ones that could be dynamic in the init-state section of the instance definition, but I don't think we care about that difference during parsing (and we check if variables are static or not automatically).

    During parsing, there is a difference (ref parser.ypp 232-239 and 571-576) and corresponding classes are used. Currently I don't see a way to merge classes but am looking into other, existing classes so that I can use them instead of either VariableSchematic or VariableInstanceSchematic.

  10. Đorđe Relić

    I managed to remove some more classes (besides VariableSchematic) and I see room to remove even more and make better use of the rddlTask pointer in the parser.ypp file. The code is cleaner now and will be even more when I continue the cleaning process next week.

  11. tkeller reporter

    I added discrete distribution and enum support to get rid of the last class in rddl.h besides RDDLTask.

    Unrelated to this, when testing for equal behaviour between rddl_parser and rddl_prefix_parser I realized that rddl_parser crashes in the wildifre domain with the following error message:

    ERROR! ExpressionList not implemented yet.
    

    @Đorđe Relić, can you look into this? Can you also make sure my merge didn't remove any of your changes of rev 262?

  12. Đorđe Relić

    Rev 261 was just aesthetical and all is fine after your merge.

    The error was actually in wildfire domain file. Exponential functions were called with exp token and you deleted that feature from lexer.l in one of earlier commits. However, I have added implementation of ExpressionList to parser even though tries of parsing it would lead to Not implemented yet error. The usage of ExpressionList is in definition of aggregative functions that are not Product, Exponential, Sum or Exists.

  13. tkeller reporter

    I tested your changes and everything seems to work now. However, it is very important that we never make any changes to the files in testbed/benchmarks/ippc2011/rddl, testbed/benchmarks/ippc2014/rddl, and testbed/benchmarks/ippc-all/rddl. All our experiments (in particular for papers we'd like to publish) are based on those files, and they must be identical to the files that were used for the IPPCs.

    I therefore made the following changes:

    • revert your change to wildfire_mdp.rddl
    • re-introduce exp in lexer.l
    • rename all benchmark folders that do not contain original IPPC domains. These were also called ippc2011-sac and ippc2011-enum because they contain altered versions of IPPC domains that are (solution) equivalent to the original domains. However, these are not the same domains and we can alter them if we need to (they have been created for testing purposees only).
  14. Log in to comment