Various CSG storage fixes#22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on fixing action/synchronisation label storage and indexing for concurrent stochastic games (CSGs), and updates/extends regression tests for CSG model export.
Changes:
- Fix concurrent-transition action retrieval/formatting in the simulator model generator and adjust action-name sources to use synchronisations consistently.
- Rework explicit CSG action handling to rely on the shared
ActionListinfrastructure (including extracting per-player actions fromJointActionwhen deriving action sets). - Add/update CSG export regression fixtures and pin previously-undefined constants in a CSG test model.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| prism/src/simulator/ModulesFileModelGenerator.java | Fixes how concurrent transition action arrays are retrieved and how action indices are rendered to strings. |
| prism/src/prism/ModelGenerator.java | Removes action-index helper methods from the ModelGenerator interface. |
| prism/src/parser/ast/ModulesFile.java | Avoids adding a null action label to the action list for concurrent games. |
| prism/src/explicit/LTLModelChecker.java | Copies actions into CSG product models via the ModelExplicit API. |
| prism/src/explicit/CSGSimple.java | Removes CSG-local action-list storage; uses shared ActionList and adjusts idle action handling. |
| prism/src/explicit/ConstructModel.java | Changes when idle actions are added during CSG construction. |
| prism/src/explicit/ChoiceActionsSimple.java | When discovering used actions, extracts per-player actions from JointAction labels. |
| prism-tests/functionality/verify/csgs/zerosum/aloha_backoff2.prism | Pins previously-undefined constants to concrete values. |
| prism-tests/functionality/export/csg/medium_access2.prism | Adds a CSG model used for export regression testing. |
| prism-tests/functionality/export/csg/medium_access2.prism.exportmodel.auto | Adds export invocation script for the medium_access2 CSG model. |
| prism-tests/functionality/export/csg/medium_access2.prism.tra | Adds expected exported transitions for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism.sta | Adds expected exported state-vector info for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism.lab | Adds expected exported labels for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism1.srew | Adds expected exported state rewards (“r1”) for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism1.trew | Adds expected exported transition rewards (“r1”) for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism2.srew | Adds expected exported state rewards (“r2”) for medium_access2. |
| prism-tests/functionality/export/csg/medium_access2.prism2.trew | Adds expected exported transition rewards (“r2”) for medium_access2. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism | Adds a CSG model used for export regression testing. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.export.auto | Adds export invocation script for the aloha_backoff2 CSG model. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.tra | Adds expected exported transitions for aloha_backoff2. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.sta | Adds expected exported state-vector info for aloha_backoff2. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.lab | Adds expected exported labels for aloha_backoff2. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.srew | Adds expected exported state rewards for aloha_backoff2. |
| prism-tests/functionality/export/csg/aloha_backoff2.prism.trew | Adds expected exported transition rewards for aloha_backoff2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| // Add idle actions | ||
| if (modelType == ModelType.CSG) |
There was a problem hiding this comment.
csg.addIdleIndexes() is executed even when justReach is true. In that mode the CSG model instance is not constructed (so csg can be null), causing a NullPointerException during reachable-state exploration for CSGs. Guard this with !justReach (or check csg != null) so reachability-only runs work for CSG models.
| if (modelType == ModelType.CSG) | |
| if (!justReach && modelType == ModelType.CSG) |
| public void addIdleIndexes() | ||
| { | ||
| int max = actions.size() + 1; | ||
| int max = getActions().size() + 1; | ||
| for (int p = 0; p < idles.length; p++) { | ||
| actions.add(max - 1, "<" + p + ">"); | ||
| actionList.addAction("<" + p + ">"); | ||
| idles[p] = max++; | ||
| } | ||
| } |
There was a problem hiding this comment.
addIdleIndexes() assumes each idle label is appended at the end by using max = getActions().size() + 1 and then incrementing. But ActionList.addAction() de-duplicates; if an action label like "<0>" already exists, addAction() will return an existing index and not append, leaving idles[p] inconsistent with the actual action index. Use the return value of actionList.addAction(...) (plus 1 for 1-indexing) when setting idles[p], or ensure generated idle labels cannot collide with user-defined actions.
| /** | ||
| * Get the action label of a transition within a choice, specified by its index/offset. | ||
| * The label can be any Object, but will often be treated as a string, so it should at least | ||
| * have a meaningful toString() method implemented. Absence of an action label is denoted by null. | ||
| * Note: For most types of models, the action label will be the same for all transitions within | ||
| * the same nondeterministic choice (i.e. for each different value of {@code offset}), | ||
| * but for Markov chains this may not necessarily be the case. | ||
| * @param i Index of the nondeterministic choice | ||
| * @param offset Index of the transition within the choice | ||
| */ | ||
| public Object getTransitionAction(int i, int offset) throws PrismException; | ||
|
|
||
| /** | ||
| * Get the action label of a transition within a choice, specified by its index/offset. | ||
| * This method provides the index of the action in the list of all actions, | ||
| * available from {@link #getActions()}. This information is optional - only | ||
| * {@link #getTransitionAction} has to be implemented, but if {@link #getActions()} | ||
| * returns non-null, then you can rely on this method to work. | ||
| * Absence of an action label is denoted by -1. | ||
| * Note: For most types of models, the action label will be the same for all transitions within | ||
| * the same nondeterministic choice (i.e. for each different value of {@code offset}), | ||
| * but for Markov chains this may not necessarily be the case. | ||
| * @param i Index of the nondeterministic choice | ||
| * @param offset Index of the transition within the choice | ||
| */ | ||
| public default int getTransitionActionIndex(int i, int offset) throws PrismException | ||
| { | ||
| List<Object> actions; | ||
| if ((actions = getActions()) != null) { | ||
| int a = actions.indexOf(getTransitionAction(i, offset)); | ||
| if (a != -1) { | ||
| return i; | ||
| } else { | ||
| throw new PrismException("Action name \"" + getTransitionAction(i, offset) + "\" is not in the list of actions"); | ||
| } | ||
| } else { | ||
| throw new PrismException("Action index information not available"); | ||
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This PR removes getTransitionActionIndex() / getChoiceActionIndex() from the public ModelGenerator interface. Even if unused internally, this is a breaking API change for downstream code that depends on these helpers. Consider keeping them (and fixing/adjusting the implementation) with a deprecation cycle, or provide an alternative supported way to obtain action indices.
Actions were off by one in CSG models that include unlabelled commands. This is because, after recent model export changes, null gets included in the action list in these cases. But CSG code current hard-codes action indices that do not take this into account. Currently unlabelled commands appear only in special cases in non-player modules, so there will never be unlabelled actions in the final CSG. For now, the list is set up differently for CSGs, but it would be preferable to later align CSGs with other models in this regard.
Not currently used; will be added in PRISM and ported in.
And the current implementation is inconsistent, since it
does not include unlabelled ("") actions in the list.
Action indices are (1-indexed) references to list of synchronising actions from the ModulesFile. Currently this coincides with getActions(), but that will not always be true.
Also update findActionsUsed to handle joint actions, although this is not currently needed since the action list for CSGs is set up front to allow action indexing. Also remove CSGSimple.getActionsForPlayer (unused, and seems broken).
Triggered by, e.g.: prism-games ../prism-examples/csgs/aloha/aloha_backoff2.prism -const bcmax=2,D=8,q=0.9 -simpath 10 stdout
No description provided.