-
Notifications
You must be signed in to change notification settings - Fork 0
Petri net refactor #21
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
Conversation
the pain, he knew! refactored weights/guards Update test_net_export.py
…alas into petri-net-refactor
adamburkegh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's mostly code moves, I think it will be much less painful if we fix the small things I've highlighted and then commit. I really don't want to end up with a snowball patch from hell.
| def weight(self,value:float) -> None: | ||
| self._weight = value | ||
|
|
||
| # data model methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically data model is a bit more of a general term, but I use this term to refer to emulating behaviour https://docs.python.org/3/reference/datamodel.html#emulating-generic-types. But the data model is very broad.
| return PetriNetFragmentParser().create_net(net_title,net_text) | ||
|
|
||
| def parse_net_fragments(net_title:str, *fragments:str) -> LabelledPetriNet: | ||
| def parse_net_fragments(net_title:str, *fragments:str) -> WeightedPetriNet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may not make sense in the same patch, but the better behaviour here would be to return a different type based on the text passed in. So if you don't included weighted transitions, you don't get a weighted net.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, what if I mix both? Keeping it simple may be easier for the future. Outside of type instance checks the wpn is functionally equivalent, so the user does not lose anything in returning a wpn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you are right that WPN is a direct subclass of PN so it is ok at the moment. It would also be good to keep it open to extension (eg a DPN) without breaking the API contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of mixing both, I guess you mean weighted and unweighted in the same net, that would not be a valid WPN though.
pmkoalas/models/petrinets/export.py
Outdated
| if isinstance(tran.tid, int): | ||
| wstring = f"Transition {tran} has an integer id, this is not " \ | ||
| +"recommended, importing into others may break." | ||
| warn(wstring) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having something allowed but spitting warnings all over I/O is irritating. It would be better not to warn, but parameterize whether to override with ProM-compatible ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you be okay with being a debug-level message or gone all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to kill the I/O side effect and the complicating runtime conditional. But it would be good to mention this integer / uuid stuff as part of a package doc comment that describes the design context. Eg how some of the important importers work and how that constrains the way people may want to use the export function. But also the advantages of simple integer ids during development and other uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, maybe we should consider that as a general tennet? Removed warns and adding some design considerations to pn.py.
|
@adamburkegh LGTY enough for a merge onto main and we deal with anything else directly? |
adamburkegh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some more small comments, another round of review seems excessive though. If they seem reasonable go ahead and push to main when ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that's my comment about utilities, but the first function is weighted net specific, and the second is not specific to pn, so maybe this isn't a win
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear instructions.
Code related to Petri nets in the repo has been broken down into separated files in a new module
petrinets. The refactor introduces the following existing structures across the following inner modules.Net types:
pn):intids when exporting.strof the id, i.e.str(n.id)is always used.preset(node)andpostset(node)return the set of preset/postset nodes based on the given inpuit.place in marking, is place marked in the markingmarking[place], returns the marking of that place or zerofor place in marking, iterates over the marked places in the markingmark + omark, joins the markings over their marked placesmark - omark, reduces all marked places in mark by their counterpart in omark, no negatives allowedmark << omark, check whethermarkis a subset ofomark.dpn)pncounterparts and update hint types:wpn)BuildablePetriNethas been moved into this file and updated to keep original behaviour of making transitions with weights.pncounterparts and update hint types:Import and Exporting pnml files:
read:parse_pnml_into_lpn, reads in aLabelledPetriNetorAcceptingPetriNetif marking information is present.parse_pnml_into_wpn, read in aWeightedPetriNetorWeightedAcceptingPetriNet-- same as aboveparse_pnml_for_dpn, read in aPetriNetWithDataorAcceptingDataPetriNet-- same as aboveexport:export_net_to_pnml, exports the given net with as much information as possible, works for all types.I have moved guard implementation for dpns to be in the same dir and moved all other existing methods in the previous
petrinet.pyfile to into eitherutils.pyordot.py. There is another file I created for prettier dot files for lpns in the upper directory, but unsure whos should be the goto.Leaving the possible refactor for reading and importing using a chain of responsibility for another day.
@adamburkegh you may wish to suggest stochastic over weighted, let me know.
While refactoring, I ensured that all previous behaviour expected by the current set of tests is passing. Updated the tests to reflect the file changes and refactoring for types of nets.