Skip to content

Add countOfPetriNodesBounds parameter to SelectPetri and MatchPetri modules#367

Merged
marcellussiegburg merged 8 commits intodevfrom
copilot/fix-294
Sep 16, 2025
Merged

Add countOfPetriNodesBounds parameter to SelectPetri and MatchPetri modules#367
marcellussiegburg merged 8 commits intodevfrom
copilot/fix-294

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 14, 2025

  • Add countOfPetriNodesBounds parameter to SelectPetri and MatchPetri source modules
  • Update example configurations to include required countOfPetriNodesBounds field
  • Remove new example configurations per reviewer feedback
  • Maintain backward compatibility with default (0, Nothing) values
  • Validate builds and tests pass
  • Apply HLint recommendations
  • Move checkCount helper function to shared Auxiliary.Util module per reviewer feedback
  • Fix build error by correcting weightedShuffle imports and exports
  • Address reviewer feedback on checkCount function argument order and formatting
  • Remove unnecessary TypeApplications extensions from MatchPetri and SelectPetri modules

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…odules

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copilot AI changed the title [WIP] control size of PetriNet after translation in AD tasks that do display PetriNets Add countOfPetriNodesBounds parameter to SelectPetri and MatchPetri modules Sep 14, 2025
Copilot AI requested a review from jvoigtlaender September 14, 2025 12:48
@jvoigtlaender
Copy link
Copy Markdown
Member

@copilot, revert your changes in the test/ directory, and also do not add completely new configurations in the example/ directory.

…eedback

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 14, 2025

@copilot, revert your changes in the test/ directory, and also do not add completely new configurations in the example/ directory.

Done. Reverted all changes in the test/ directory and removed the new configurations from the example/ directory while preserving the core functionality in the source modules. Commit 529c036.

@jvoigtlaender
Copy link
Copy Markdown
Member

@copilot, I had asked you to remove the

new configurations

in the example/ directory, not to completely revert your changes there.

Indeed, the countOfPetriNodesBounds field additions are, of course, needed!

…rations

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 14, 2025

@copilot, I had asked you to remove the

new configurations

...

You're absolutely right. I've restored the required countOfPetriNodesBounds = (0, Nothing) field additions to all existing configurations while keeping the new example configurations removed. The field is needed for compilation since it's now part of the data types. Commit 206a1e9.

@jvoigtlaender
Copy link
Copy Markdown
Member

@copilot, move the three occurrences of the checkCount helper function into one location from where it is imported to where it is needed.

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 14, 2025

@copilot, move the three occurrences of the checkCount helper function into one location from where it is imported to where it is needed.

Done. Moved the three checkCount helper functions to Modelling.ActivityDiagram.Auxiliary.Util where they can be imported by all modules that need them. The function is now defined once and shared across MatchPetri, SelectPetri, and FindAuxiliaryPetriNodes modules. Commit d86ba07.

@jvoigtlaender
Copy link
Copy Markdown
Member

@copilot, fix the build error

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Sep 14, 2025

@copilot, fix the build error

Fixed the build error. The issue was that weightedShuffle was being exported from Modelling.ActivityDiagram.Auxiliary.Util but not actually defined there. Removed it from the exports and cleaned up unused imports since weightedShuffle is already correctly imported from Modelling.Auxiliary.Common in the modules that use it. Build now passes successfully. Commit 030d7ed.

@jvoigtlaender jvoigtlaender marked this pull request as ready for review September 14, 2025 16:30
@jvoigtlaender
Copy link
Copy Markdown
Member

@marcellussiegburg, does this appropriately address #294?

@jvoigtlaender
Copy link
Copy Markdown
Member

There is probably some interaction with #375.

-- | Check if the count of Petri nodes in a converted activity diagram
-- falls within the given bounds
checkCount :: UMLActivityDiagram -> (Int, Maybe Int) -> Bool
checkCount ad countOfPetriNodesBounds =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The flipped order of arguments would be more natural?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably so, yes. I would ask @copilot to address this as well as your other comments. Unfortunately, it seems it cannot read comment contents directly. So maybe it's more bothersome (copying the comments into a new one in the main thread, along with indications targetting the files and line numbers) than if you simply make the edits directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Flipped the argument order to make checkCount :: (Int, Maybe Int) -> UMLActivityDiagram -> Bool which is more natural. Updated all call sites accordingly. Commit 65b92ca.

checkCount :: UMLActivityDiagram -> (Int, Maybe Int) -> Bool
checkCount ad countOfPetriNodesBounds =
let count = M.size . Petri.nodes @PetriLike @SimpleNode
$ convertToPetriNet ad in
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird formatting of in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed the weird formatting of in keyword. Now properly aligned with better indentation. Commit 65b92ca.

{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE TypeApplications #-}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TypeApplications are not required here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, removed the unnecessary TypeApplications extension from MatchPetri.hs since it's not used in this module. Commit 65b92ca.

{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TypeApplications #-}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TypeApplications are not required here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, removed the unnecessary TypeApplications extension from SelectPetri.hs since it's not used in this module. Commit 65b92ca.

@marcellussiegburg
Copy link
Copy Markdown
Member

@marcellussiegburg, does this appropriately address #294?

Yes, it does.

…ve unused TypeApplications

Co-authored-by: jvoigtlaender <5853832+jvoigtlaender@users.noreply.github.com>
@jvoigtlaender
Copy link
Copy Markdown
Member

Hah, seems like Copilot is able to do that (directly working with reviewer comments) after all.

@marcellussiegburg marcellussiegburg merged commit 0930d24 into dev Sep 16, 2025
16 checks passed
@marcellussiegburg marcellussiegburg deleted the copilot/fix-294 branch September 16, 2025 14:25
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.

control size of PetriNet after translation in AD tasks that do display PetriNets

3 participants