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

Tracing improvements #1224

Merged
merged 9 commits into from
Dec 17, 2024
Merged

Tracing improvements #1224

merged 9 commits into from
Dec 17, 2024

Conversation

rocky
Copy link
Member

@rocky rocky commented Dec 14, 2024

Trace rewrite rules. Also, we were dropping some return value expressions.

Improvement seen from mathics:

In[1]:= TraceEvaluation[Exponent[x, 1]]
  Evaluating Expression.evaluate(): System`Exponent[Global`x, 1]
    Rewriting: System`Exponent[Global`x, 1, System`Max] -> (<Expression: <Symbol: System`Exponent>[<Symbol: Global`x>, <Integer: 1>, <Symbol: System`Max>]>, True)
    -> System`Exponent[Global`x, 1, System`Max]
    Rewriting: System`Max[0] -> (<Expression: <Symbol: System`Max>[<Integer: 0>]>, True)
    -> System`Max[0]
  Returning: System`Exponent[Global`x, 1] = 0
Out[1]= 0

In[2]:= TraceEvaluation[1+3]
  Evaluating Expression.evaluate(): System`Plus[1, 3]
  Returning: System`Plus[1, 3] = 4
Out[2]= 4

image

also, we were dropping some return value expression
@rocky rocky requested a review from mmatera December 14, 2024 08:32
@rocky rocky force-pushed the tracing-improvements branch from db45626 to 6005173 Compare December 14, 2024 19:03
* Show return values
* Show rewrite rules
@rocky rocky force-pushed the tracing-improvements branch from 6005173 to ed58267 Compare December 14, 2024 19:32
if evaluation.definitions.timing_trace_evaluation:
evaluation.print_out(time.time() - evaluation.start_time)

# Test and dispose of various situations where showing information
# is pretty useless: evaluating a Symbol is the Symbol.
Copy link
Contributor

Choose a reason for hiding this comment

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

mmm what about the ownvalues? (Example: a=3 and then a?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. But better, if you have a suggestion for how to improve, just commit on top. There is a lot of room for improvement. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is just that I do not understand yours: differently to what happens with an Integer or a String, Symbols can be evaluated to something different. It happens when they definitions contain Downvalues.

Copy link
Member Author

@rocky rocky Dec 14, 2024

Choose a reason for hiding this comment

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

As a heuristic, I have noticed that if we miss something like this, that is if we miss a transition from a symbol not having a value to one new getting a value, that is generally okay because the value starts to show up in the next evaluate or return message.

Right now, in my view TraceEvaluation is a little too noisy. Down the line, I imagine a way to indicate whether one wants to see both rewrites and evaluations or just one or the other.

On the debugger side I image even more flexibility such as filtering on specific kinds of condition (recursion level is in a particular range, or the evaluations to show are only from a limited set, etc.)

What I suggest here, is to come up try out what is here and come up with specific examples and output and then show how one is better or worse than another. And then also if you want to modify code to show how things get better, that is great too!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I get in master:

In[1]:= a=4; 
Out[1]= None

In[2]:= TraceEvaluation[a+b]
  Evaluating: System`Plus[Global`a, Global`b]
    Evaluating: System`Plus
    Evaluating: Global`a
      -> 4
    Evaluating: Global`b
Out[2]= 4 + b

and this with this branch:

In[1]:= a=4;
Out[1]= None

In[2]:= TraceEvaluation[a+b]
  Evaluating Expression.evaluate(): System`Plus[Global`a, Global`b]
      -> 4
    Returning: Global`a = 4
  Returning: System`Plus[Global`a, Global`b] = System`Plus[4, Global`b]

The difference that I see is that trivial evaluations of the expression heads are not shown. I guess that is OK. However, I find

  Evaluating Expression.evaluate(): System`Plus[Global`a, Global`b]
      -> 4

a little bit confusing: it looks like the result of evaluating Plus[a,b] is 4. Then
Returning: Global`a = 4 only makes sense if we assume that there was a call to Symbol.evaluate().

Copy link
Member Author

@rocky rocky Dec 15, 2024

Choose a reason for hiding this comment

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

The difference that I see is that trivial evaluations of the expression heads are not shown. I guess that is OK. However, I find

  Evaluating Expression.evaluate(): System`Plus[Global`a, Global`b]
      -> 4

a little bit confusing

This apparently was old output from the code inside Expression.evaluation for old way TraceEvaluation was implemented. I removed that code in commit d6e301d

@rocky rocky force-pushed the tracing-improvements branch from 581db62 to 57409f0 Compare December 16, 2024 01:34
Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

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

It seems that this simplifies the code, and also helps to make that the trepan output looks better, so LGTM.

@rocky rocky merged commit 59a20e4 into master Dec 17, 2024
13 checks passed
@rocky rocky deleted the tracing-improvements branch December 17, 2024 02:24
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.

2 participants