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

Simplified generator system #657

Merged
merged 34 commits into from
Jul 19, 2021
Merged

Simplified generator system #657

merged 34 commits into from
Jul 19, 2021

Conversation

maxitg
Copy link
Owner

@maxitg maxitg commented Jun 16, 2021

Changes

  • This is a complete rewrite of the generator system.

  • Old GenerateMultihistory syntax is removed.

  • Instead, all generators have the format

    Generator[System[rules], param1 -> value1, ...] @ init

    Note that param1, etc. are symbols rather than strings. They have usage messages, etc. Also, one can use lists or associations instead, e.g.,

    Generator[System[rules], {param1 -> value1}, <|param2 -> value2|>, param3 -> value3] @ init
  • Parameters are now declared separately from systems and generators.

  • Systems need to declare a logical expression specifying which parameters can be specified. For example,

    Implies[MaxEvents || MaxDestroyerEvents, EventOrder]

    means that EventOrder needs to be specified if either MaxEvents or MaxDestroyerEvents are specified.

  • Generators have predefined values for some parameters. E.g., GenerateSingleHistory sets MaxDestroyerEvents -> 1, which can no longer be changed.

Comments

  • Apologies for a huge PR. There is a lot of refactoring here as all instances of GenerateMultihistory had to be changed.
  • Ordering functions page is deleted for now but should return as a page for the EventOrder parameter once it is used somewhere.
  • @daneelsan, unfortunately, it will break HypergraphSubstitutionSystem #643, but on the flip side, it should make it a lot easier to define parameters (as one does not need to think where to put them anymore).

Examples

  • GenerateSingleHistory:
In[] := #["StatesList"] & @
 SetReplaceTypeConvert[{WolframModelEvolutionObject, 2}] @
  GenerateSingleHistory[MultisetSubstitutionSystem[{a_, b_} :> {a + b}], MaxEvents -> 4] @ {1, 2, 3}
Out[] = {{1, 2, 3}, {3, 3}, {6}}
  • GenerateAllHistories:
In[] := #["ExpressionsEventsGraph", VertexLabels -> Placed[Automatic, After]] & @
 SetReplaceTypeConvert[{WolframModelEvolutionObject, 2}] @
  GenerateMultihistory[
    MultisetSubstitutionSystem[{a_, b_} /; a < b :> {a + b}], MaxGeneration -> 2] @ Range[3]

image


This change is Reviewable

@maxitg maxitg added evolution Modifies code for running the evolution of the model convenience Syntax improvements that don't significantly change functionality wolfram language Requires Wolfram Language implementation labels Jun 16, 2021
@maxitg maxitg added this to the Project Yellowstone milestone Jun 16, 2021
@maxitg maxitg changed the title GenerateFullMultihistory and GenerateSingleHistory GenerateAllHistories and GenerateSingleHistory Jun 17, 2021
@maxitg maxitg force-pushed the feature/shorthandGenerators branch from 2a9e95d to 8df927c Compare June 17, 2021 12:52
@maxitg maxitg force-pushed the feature/shorthandGenerators branch from 8df927c to 3679dc9 Compare July 8, 2021 15:23
@maxitg maxitg changed the title GenerateAllHistories and GenerateSingleHistory Simplified generator system Jul 8, 2021
@maxitg maxitg added the breaking Introduces changes that could break existing code label Jul 11, 2021
Base automatically changed from feature/autoInputCounting to master July 11, 2021 22:50
@maxitg maxitg force-pushed the feature/shorthandGenerators branch from d378644 to 45f5d3e Compare July 11, 2021 22:54
@maxitg maxitg added refactor Does not change functionality, but makes the code better organized or more readable and removed convenience Syntax improvements that don't significantly change functionality labels Jul 12, 2021
@maxitg maxitg self-assigned this Jul 12, 2021
@maxitg maxitg marked this pull request as ready for review July 12, 2021 00:40
@maxitg maxitg requested review from daneelsan and taliesinb July 12, 2021 00:40
@maxitg maxitg mentioned this pull request Jul 13, 2021
Copy link
Collaborator

@daneelsan daneelsan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 7 unresolved discussions (waiting on @maxitg and @taliesinb)


Documentation/Generators/MaxEventInputs.md, line 3 at r2 (raw file):

the max and min

I prefer the maximum and minimum


Documentation/Generators/MaxEventInputs.md, line 9 at r2 (raw file):

However, if the range of match sizes is known ahead of time, one can set it explicitly.

... one can set it explicitly, to reduce/minimize the number of matchings generated


Documentation/Generators/MaxGeneration.md, line 7 at r2 (raw file):

The generation of an event is defined as the maximum of the generations of its inputs plus one.

This means that matchings instantiations are sorted by the generations of the inputs? That is, I can't instantiate a match that has an input of generation 3, if there still remains a match with an input of generation 2.


Documentation/Generators/MaxGeneration.md, line 29 at r2 (raw file):

Quoted 7 lines of code…
In[] := #["ExpressionsEventsGraph",
          VertexLabels -> Placed["Name", After, Replace[{{"Expression", n_} :> #["ExpressionGenerations"][[n]],
                                                         {"Event", n_} :> #["EventGenerations"][[n]]}]]] & @
  SetReplaceTypeConvert[{WolframModelEvolutionObject, 2}] @
    GenerateMultihistory[MultisetSubstitutionSystem[{a__} /; Total[{a}] == 5 :> {Total[{a}] - 1, Total[{a}] + 1}],
                         {1, 2, 3},
                         MaxEvents -> 3]

We need a VertexLabels -> "Generation"


Documentation/Generators/MaxGeneration.md, line 38 at r2 (raw file):

Since `MaxGeneration` is a selection parameter rather than a stopping condition, it will continue evaluation even after
encountering matches exceeding the generations constraint, which might also result in a different
event order (i.e., some events being skipped) than if using, e.g., [`MaxEvents`](MaxEvents.md). For this reason,

Something like:
"MaxGeneration is an event (match?) selection parameter, not a stopping condition. That is, the evolution of the system won't stop
if a match with a generation greater than the constraint is encountered. Rather, it will ignore those matches and instantiate only those
that satisfy the generation constraint. This selection of matching (events?) according to their generation will (in most cases) affect the
ordering of events, as opposed to using stopping conditions, e.g. MaxEvents".

PS: I'm unsure how to call the "generation of a match". "Generation index" of a match?


Documentation/Generators/README.md, line 13 at r2 (raw file):

which is essential when the system does not terminate

Why is it essential?


Documentation/Generators/README.md, line 48 at r2 (raw file):

```wl
Generator[System[rules], init, parameters...]

Why did you decide to support this syntax?

Copy link
Collaborator

@daneelsan daneelsan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 10 unresolved discussions (waiting on @maxitg and @taliesinb)


Kernel/A1$generatorSystem.m, line 26 at r2 (raw file):

Parameters in the fourth argument should be declared with declareSystemParameter.

So, call declareSystemParameter before declareSystem


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

                   _ ? FailureQ,
                   message[generator, #, <|"expr" -> HoldForm[expr]|>] &];
    result /; !FailureQ[result]

Recently, Leonid gave a couple of talks about error handling in WL. He wen't over almost all conventions, and concluded that messages and unevaluated expressions are really not the way to go. Leaving the input unevaluated was fine when we dealt with symbolic manipulation, which is not the case here. Messages should probably only be used to issue warnings, not errors. Don't you think a workflow that only deals with Failure objects would be easier to implement and design?


Kernel/systemParameters.m, line 26 at r2 (raw file):

(* Stopping-condition parameters *)

PackageExport["MaxEvents"]

So now Package* are no longer required to be at the top of the file? 👀

Copy link
Owner Author

@maxitg maxitg left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 10 unresolved discussions (waiting on @daneelsan and @taliesinb)


Documentation/Generators/MaxEventInputs.md, line 3 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
the max and min

I prefer the maximum and minimum

Done.


Documentation/Generators/MaxEventInputs.md, line 9 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
However, if the range of match sizes is known ahead of time, one can set it explicitly.

... one can set it explicitly, to reduce/minimize the number of matchings generated

Done.


Documentation/Generators/MaxGeneration.md, line 7 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
The generation of an event is defined as the maximum of the generations of its inputs plus one.

This means that matchings instantiations are sorted by the generations of the inputs? That is, I can't instantiate a match that has an input of generation 3, if there still remains a match with an input of generation 2.

Not if events are independent:

In[] := Normal @ GenerateSingleHistory[
  MultisetSubstitutionSystem[{{1} -> {2}, {2} -> {5}, {3, 4} -> {6}}], {1, 2, 3, 4}][[2, "EventGenerations"]]
Out[] = {0, 1, 1, 2, 1}

Or, by explicitly setting the event order:

In[] := WolframModel[{{1, 2}, {2, 3}} -> {{1, 3}},
                     {{a, b}, {b, c}, {1, 2}, {2, 3}, {3, 4}},
                     Infinity,
                     "EventOrderingFunction" -> {"NewestEdge"}]["EventGenerations"]
Out[] = {1, 2, 1}

"EventOrderingFunction" -> Automatic has this property though by design.


Documentation/Generators/MaxGeneration.md, line 29 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
In[] := #["ExpressionsEventsGraph",
          VertexLabels -> Placed["Name", After, Replace[{{"Expression", n_} :> #["ExpressionGenerations"][[n]],
                                                         {"Event", n_} :> #["EventGenerations"][[n]]}]]] & @
  SetReplaceTypeConvert[{WolframModelEvolutionObject, 2}] @
    GenerateMultihistory[MultisetSubstitutionSystem[{a__} /; Total[{a}] == 5 :> {Total[{a}] - 1, Total[{a}] + 1}],
                         {1, 2, 3},
                         MaxEvents -> 3]

We need a VertexLabels -> "Generation"

I was just working on it yesterday:

In[] := TokenEventGraph[VertexLabels -> (#Generation &)] @
  GenerateMultihistory[MultisetSubstitutionSystem[{a__} /; Total[{a}] == 5 :> {Total[{a}] - 1, Total[{a}] + 1}],
                       {1, 2, 3},
                       MaxEvents -> 3]

Still trying to figure out what the API should be.


Documentation/Generators/MaxGeneration.md, line 38 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
Since `MaxGeneration` is a selection parameter rather than a stopping condition, it will continue evaluation even after
encountering matches exceeding the generations constraint, which might also result in a different
event order (i.e., some events being skipped) than if using, e.g., [`MaxEvents`](MaxEvents.md). For this reason,

Something like:
"MaxGeneration is an event (match?) selection parameter, not a stopping condition. That is, the evolution of the system won't stop
if a match with a generation greater than the constraint is encountered. Rather, it will ignore those matches and instantiate only those
that satisfy the generation constraint. This selection of matching (events?) according to their generation will (in most cases) affect the
ordering of events, as opposed to using stopping conditions, e.g. MaxEvents".

PS: I'm unsure how to call the "generation of a match". "Generation index" of a match?

Yes, that is much more clear. What's wrong with "generation of a match"? A match is just a "tentative" event, it has all the same properties except for output tokens.


Documentation/Generators/README.md, line 13 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
which is essential when the system does not terminate

Why is it essential?

Because if the system does not terminate and no parameters are specified, it's going to run forever. I clarified the explanation a bit.


Documentation/Generators/README.md, line 48 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

Why did you decide to support this syntax?

It just seemed awkward to use operator form in cases where the init is very small. Especially in cases like these:

GenerateMultihistory[MultisetSubstitutionSystem[{{} -> {1, 2}, {a_, b_} :> {a + b}}]] @ {}

However, it does cause problems as the parameters might get interpreted as an init if the pattern for that init is too general (like in the case of AtomicStateSystem). So what do you think, should I remove it?


Kernel/A1$generatorSystem.m, line 26 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…
Parameters in the fourth argument should be declared with declareSystemParameter.

So, call declareSystemParameter before declareSystem

They are called in separate files, but I reordered the definitions in this file if that's what you meant.


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

Recently, Leonid gave a couple of talks about error handling in WL. He wen't over almost all conventions, and concluded that messages and unevaluated expressions are really not the way to go. Leaving the input unevaluated was fine when we dealt with symbolic manipulation, which is not the case here. Messages should probably only be used to issue warnings, not errors. Don't you think a workflow that only deals with Failure objects would be easier to implement and design?

Yes, that does seem like a good idea. All other languages seem to be doing this as well. In fact, that's what we are doing internally precisely because it's more convenient. It might also be helpful outside of the package. Plus, it will indeed simplify things a lot because we won't need to keep two separate versions for every function.

Although I think instead of returning Failure objects, we should throw exceptions.

It's very unconventional in WL, but I don't see what the downsides would be. @taliesinb, what do you think?

PS. Not in this PR, though, as we need to make it consistent across all functions.


Kernel/systemParameters.m, line 26 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

So now Package* are no longer required to be at the top of the file? 👀

Good catch! I moved it to the top.

@maxitg maxitg requested a review from daneelsan July 15, 2021 17:24
Copy link
Collaborator

@daneelsan daneelsan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 6 unresolved discussions (waiting on @maxitg and @taliesinb)


Documentation/Generators/MaxGeneration.md, line 7 at r2 (raw file):

Normal @ GenerateSingleHistory[
MultisetSubstitutionSystem[{{1} -> {2}, {2} -> {5}, {3, 4} -> {6}}], {1, 2, 3, 4}][[2, "EventGenerations"]]
So it instantiates any match that has a less-or-equal than the max generation constraint, not necessarily following a low-to-high generation order. That makes sense.


Documentation/Generators/MaxGeneration.md, line 29 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

I was just working on it yesterday:

In[] := TokenEventGraph[VertexLabels -> (#Generation &)] @
  GenerateMultihistory[MultisetSubstitutionSystem[{a__} /; Total[{a}] == 5 :> {Total[{a}] - 1, Total[{a}] + 1}],
                       {1, 2, 3},
                       MaxEvents -> 3]

Still trying to figure out what the API should be.

Interesting... The user is filled with power


Documentation/Generators/MaxGeneration.md, line 38 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

Yes, that is much more clear. What's wrong with "generation of a match"? A match is just a "tentative" event, it has all the same properties except for output tokens.

I'm just unsure whether we've explained what the difference between a match and an event is. I like "tentative" event. TODO event, maybe?


Documentation/Generators/README.md, line 48 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

It just seemed awkward to use operator form in cases where the init is very small. Especially in cases like these:

GenerateMultihistory[MultisetSubstitutionSystem[{{} -> {1, 2}, {a_, b_} :> {a + b}}]] @ {}

However, it does cause problems as the parameters might get interpreted as an init if the pattern for that init is too general (like in the case of AtomicStateSystem). So what do you think, should I remove it?

I think we should go for the easier thing to implement. I like the idea of there only being one way to generate a multihistory: init // Generator[...]. It clearly separates the rule and the system parameters from a specific initial condition. Ideally Generator[...] should parse and process its contents without the need of specifying an initial condition, so {init1, init2} // Map[Generator[...]] only process Generator once, but I'm unsure how to go about it.


Kernel/A1$generatorSystem.m, line 26 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

They are called in separate files, but I reordered the definitions in this file if that's what you meant.

I actually meant that to define a system you'd have to first call defineSystemParameter, but now I get it.


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

Yes, that does seem like a good idea. All other languages seem to be doing this as well. In fact, that's what we are doing internally precisely because it's more convenient. It might also be helpful outside of the package. Plus, it will indeed simplify things a lot because we won't need to keep two separate versions for every function.

Although I think instead of returning Failure objects, we should throw exceptions.

It's very unconventional in WL, but I don't see what the downsides would be. @taliesinb, what do you think?

PS. Not in this PR, though, as we need to make it consistent across all functions.

If by throwing you mean throwing and not catching, I'm unsure 🤔

I agree this is not the PR to do that (it would be much bigger than it already is!)

Copy link
Owner Author

@maxitg maxitg left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 6 unresolved discussions (waiting on @daneelsan and @taliesinb)


Documentation/Generators/MaxGeneration.md, line 7 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

Normal @ GenerateSingleHistory[
MultisetSubstitutionSystem[{{1} -> {2}, {2} -> {5}, {3, 4} -> {6}}], {1, 2, 3, 4}][[2, "EventGenerations"]]
So it instantiates any match that has a less-or-equal than the max generation constraint, not necessarily following a low-to-high generation order. That makes sense.

Yes, exactly.


Documentation/Generators/MaxGeneration.md, line 29 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

Interesting... The user is filled with power

Done.


Documentation/Generators/MaxGeneration.md, line 38 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

I'm just unsure whether we've explained what the difference between a match and an event is. I like "tentative" event. TODO event, maybe?

Ok, added "tentative event" as clarification.


Documentation/Generators/README.md, line 48 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

I think we should go for the easier thing to implement. I like the idea of there only being one way to generate a multihistory: init // Generator[...]. It clearly separates the rule and the system parameters from a specific initial condition. Ideally Generator[...] should parse and process its contents without the need of specifying an initial condition, so {init1, init2} // Map[Generator[...]] only process Generator once, but I'm unsure how to go about it.

Ok, I removed it for now, mainly because parameters can be unintentionally matched to inits. And it seems that it is more natural to think about parameters before the init, so the order of arguments in the operator form seems to make more sense.

It still seems more convenient to use the non-operator form in tests, but it will be the reverse once we allow existing multihistories to be continued.

Also, what kind of preprocessing do you think it should do?


Kernel/A1$generatorSystem.m, line 26 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

I actually meant that to define a system you'd have to first call defineSystemParameter, but now I get it.

No, the order does not matter because all non-trivial stuff happens in initializeSystemGenerators and in calls to functions.


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

If by throwing you mean throwing and not catching, I'm unsure 🤔

I agree this is not the PR to do that (it would be much bigger than it already is!)

Here is why it might be a good idea to throw and not catch. Suppose someone wants to implement a function that takes multiset rules as an argument. Of course, they would not want to implement a check themselves, so in the current API, they might do something like this:

runOnRange::invalidMultisetRules = "Rules `1` are invalid.";

runOnRange[rules_] := ModuleScope[
  result = Quiet[
    Check[GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10],
          $Failed,
          GenerateSingleHistory::invalidMultisetRules],
    GenerateSingleHistory::invalidMultisetRules];
  If[result === $Failed, Message[runOnRange::invalidMultisetRules, rules]];
  Normal @ (#[[2, "Expressions"]] &) @ result /; result =!= $Failed
];

Pretty complicated. If we return a Failure, it becomes a bit better:

runOnRange[rules_] := ModuleScope[
  result = GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10];
  If[FailureQ[result], result, Normal @ (#[[2, "Expressions"]] &) @ result]
];

However, if we throw, we don't need to do anything at all:

runOnRange[rules_] :=
  Normal @ (#[[2, "Expressions"]] &) @ GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10];

since the evaluation is going to stop immediately once the throw happens. One case I can think of where this might be unwanted is if one does a large enumeration of, say, rules and does not care if a few of them fail because the goal is to find something, and getting all results is not important. However, I would argue that one should be deliberate in these cases, and it is very easy to fix it by doing a Catch @ in front of an exception-throwing function.

@maxitg maxitg requested a review from daneelsan July 16, 2021 17:50
Copy link
Collaborator

@daneelsan daneelsan left a comment

Choose a reason for hiding this comment

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

Looks good to me. I now understand how much Generator[System[rules], param1 -> value1, ...] @ init reduces the time we spend typing.

Reviewable status: 0 of 34 files reviewed, 2 unresolved discussions (waiting on @maxitg and @taliesinb)


Documentation/Generators/README.md, line 48 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

Ok, I removed it for now, mainly because parameters can be unintentionally matched to inits. And it seems that it is more natural to think about parameters before the init, so the order of arguments in the operator form seems to make more sense.

It still seems more convenient to use the non-operator form in tests, but it will be the reverse once we allow existing multihistories to be continued.

Also, what kind of preprocessing do you think it should do?

The idea I had in mind was to use no entry to avoid parsing the arguments every time:

In[]:= ClearAll[f]
f[x_] ? System`Private`HoldEntryQ := (
   Print["Processing x..."];
   If[x > 0,
    System`Private`ConstructNoEntry[f, # + x &],
    $Failed
    ]
   );

In[]:= func = f[3]
During evaluation of In[141]:= Processing x...
Out[]= f[#1 + 3 &]

In[]:= func
Out[]= f[#1 + 3 &]

But now that I think about it, if the mapped function is not a pure function then it will be processed only once:

In[]:= g[3] /@ {1, 2, 3}
During evaluation of In[148]:= Processing x...
Out[]= {g[#1 + 3 &][1], g[#1 + 3 &][2], g[#1 + 3 &][3]}

So it doesn't matter. I think NoEntry will be useful when the head is the same as the constructor, e.g. Graph, SparseArray, etc.


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

Previously, maxitg (Max Piskunov) wrote…

Here is why it might be a good idea to throw and not catch. Suppose someone wants to implement a function that takes multiset rules as an argument. Of course, they would not want to implement a check themselves, so in the current API, they might do something like this:

runOnRange::invalidMultisetRules = "Rules `1` are invalid.";

runOnRange[rules_] := ModuleScope[
  result = Quiet[
    Check[GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10],
          $Failed,
          GenerateSingleHistory::invalidMultisetRules],
    GenerateSingleHistory::invalidMultisetRules];
  If[result === $Failed, Message[runOnRange::invalidMultisetRules, rules]];
  Normal @ (#[[2, "Expressions"]] &) @ result /; result =!= $Failed
];

Pretty complicated. If we return a Failure, it becomes a bit better:

runOnRange[rules_] := ModuleScope[
  result = GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10];
  If[FailureQ[result], result, Normal @ (#[[2, "Expressions"]] &) @ result]
];

However, if we throw, we don't need to do anything at all:

runOnRange[rules_] :=
  Normal @ (#[[2, "Expressions"]] &) @ GenerateSingleHistory[MultisetSubstitutionSystem[rules]] @ Range[10];

since the evaluation is going to stop immediately once the throw happens. One case I can think of where this might be unwanted is if one does a large enumeration of, say, rules and does not care if a few of them fail because the goal is to find something, and getting all results is not important. However, I would argue that one should be deliberate in these cases, and it is very easy to fix it by doing a Catch @ in front of an exception-throwing function.

Definitely, the first check sucks.

I like the second option, it's explicit. Although this will appear everywhere: result = ...; If[FailureQ[result], result, func[result]]

I've been playing with Confirm/Enclose quite a bit. Using those, it would look like:

runOnRange[rules_] :=
  Enclose[Normal @ (#[[2, "Expressions"]] &) @ Confirm[GenerateSingleHistory[MultisetSubstitutionSystem[rules]]] @ Range[10], "Expression"]

There are definitely some issues with the confirm/enclose approach. For instance, there is not a explicit way of throwing something, i.e. Throw[failure].
You have to do Confirm[$Failed, failure]. Also, I remember an email thread comparing the performance of confirm/enclose vs catch/throw and return, and confirm/enclose was slower. I think it's to be expected as confirm/enclose is a wrapper over catch/throw...

Copy link
Owner Author

@maxitg maxitg left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 34 files reviewed, 2 unresolved discussions (waiting on @daneelsan and @taliesinb)


Documentation/Generators/README.md, line 48 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

The idea I had in mind was to use no entry to avoid parsing the arguments every time:

In[]:= ClearAll[f]
f[x_] ? System`Private`HoldEntryQ := (
   Print["Processing x..."];
   If[x > 0,
    System`Private`ConstructNoEntry[f, # + x &],
    $Failed
    ]
   );

In[]:= func = f[3]
During evaluation of In[141]:= Processing x...
Out[]= f[#1 + 3 &]

In[]:= func
Out[]= f[#1 + 3 &]

But now that I think about it, if the mapped function is not a pure function then it will be processed only once:

In[]:= g[3] /@ {1, 2, 3}
During evaluation of In[148]:= Processing x...
Out[]= {g[#1 + 3 &][1], g[#1 + 3 &][2], g[#1 + 3 &][3]}

So it doesn't matter. I think NoEntry will be useful when the head is the same as the constructor, e.g. Graph, SparseArray, etc.

Yes, in this case, it's not processed at all until the operator has an argument. And yes, NoEntry should be useful in cases like Hypergraph.


Kernel/A1$generatorSystem.m, line 183 at r2 (raw file):

Previously, daneelsan (Daniel Sanchez) wrote…

Definitely, the first check sucks.

I like the second option, it's explicit. Although this will appear everywhere: result = ...; If[FailureQ[result], result, func[result]]

I've been playing with Confirm/Enclose quite a bit. Using those, it would look like:

runOnRange[rules_] :=
  Enclose[Normal @ (#[[2, "Expressions"]] &) @ Confirm[GenerateSingleHistory[MultisetSubstitutionSystem[rules]]] @ Range[10], "Expression"]

There are definitely some issues with the confirm/enclose approach. For instance, there is not a explicit way of throwing something, i.e. Throw[failure].
You have to do Confirm[$Failed, failure]. Also, I remember an email thread comparing the performance of confirm/enclose vs catch/throw and return, and confirm/enclose was slower. I think it's to be expected as confirm/enclose is a wrapper over catch/throw...

What are the downsides of throwing?

@maxitg maxitg merged commit 6802e3a into master Jul 19, 2021
@maxitg maxitg deleted the feature/shorthandGenerators branch July 19, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces changes that could break existing code evolution Modifies code for running the evolution of the model refactor Does not change functionality, but makes the code better organized or more readable wolfram language Requires Wolfram Language implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants