Opened 12 years ago
Last modified 4 years ago
#9704 needs_review enhancement
refactor "trace"  make trace command call trace method on input if available
Reported by:  William Stein  Owned by:  Jason Grout 

Priority:  minor  Milestone:  sageduplicate/invalid/wontfix 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  William Stein, Jason Grout, Kwankyu Lee  Reviewers:  Robert Miller, Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  u/klee/9704 (Commits, GitHub, GitLab)  Commit:  55edfa865f08bd173905dae865fae6cd4e3e2aa2 
Dependencies:  Stopgaps: 
Description (last modified by )
This is confusing. Refactor to fix it:
sage: det(matrix(2,[1,2,3,4])) 2 sage: trace(matrix(2,[1,2,3,4])) Traceback (most recent call last) ... AttributeError: 'sage.matrix.matrix_integer_dense.Matrix_integer_dense' object has no attribute 'lstrip'
Attachments (2)
Change History (35)
Changed 12 years ago by
Attachment:  trac_9704trace.patch added 

comment:1 Changed 12 years ago by
Status:  new → needs_review 

comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
I would just say leave it like it is except to add a deprecation warning to use "trace_execution" in the string case. That way we get something that is backward compatible.
comment:4 followup: 5 Changed 12 years ago by
 jason  trace already means two different things, at least. I'm not adding a new meaning.
 regarding adding a deprecation warning, I think that is reasonable, but that should not go in this ticket. If you want to do that, make it another ticket and do it.
comment:5 Changed 12 years ago by
Replying to was:
 jason  trace already means two different things, at least. I'm not adding a new meaning.
When I read the code, trace() has one purpose, not two. Your patch will mean that the trace function has two completely different purposes, depending on the input. That is what I'm 1 on.
If we change the trace() function so that it instead computes the trace of an object (i.e., x.trace()), then I agree that for a short amount of time, a deprecation warning should be added and trace() will have to serve two purposes.
 regarding adding a deprecation warning, I think that is reasonable, but that should not go in this ticket. If you want to do that, make it another ticket and do it.
I'm 1 on a ticket (this one) which makes the Sage trace() function have two purposes (unless it's a temporary thing that is deprecated, of course).
comment:6 followup: 7 Changed 12 years ago by
Sage already has _lots_ of functions f() which do very different things depending on the type of the arguments.
How about making the trace(x) function do what others do, which is to try returning x.trace() and if that fails do what the code_trace function does?
comment:7 Changed 12 years ago by
Replying to cremona:
Sage already has _lots_ of functions f() which do very different things depending on the type of the arguments.
How about making the trace(x) function do what others do, which is to try returning x.trace() and if that fails do what the code_trace function does?
Is there any other examples in Sage where a function does:
 mathematically meaningful stuff (which may differ, depending on the mathematical object), and returns a mathematical answer
 and also does something which is entirely nonmathematical, on a completely different level (a programming nutsandbolts debugging level, rather than a math level)?
The big conceptual difference between those two purposes is why I think having two functions, say trace()
(which calls x.trace()
) and trace_execution()
(which does what trace does right now) is a much better design than lumping things into one function.
comment:8 followup: 9 Changed 12 years ago by
Look Jason, this "trace" having two meanings is *already* in Sage. Whether or not that changes is orthogonal to this ticket. You could post a patch *after* this ticket gets in later if you're really worried.
English has words with different meanings. Sorry. It's just the way the language works.
comment:9 Changed 12 years ago by
Replying to was:
Look Jason, this "trace" having two meanings is *already* in Sage.
No, it doesn't at the top level, and not in the same function.
Whether or not that changes is orthogonal to this ticket. You could post a patch *after* this ticket gets in later if you're really worried.
I'm posting one right now.
English has words with different meanings. Sorry. It's just the way the language works.
Sure, but that doesn't excuse a bad design.
comment:10 Changed 12 years ago by
My patch may cause some doctest somewhere to fail because of deprecation warnings. I've tested the misc/*.py and matrix/*.py* files (and only got errors that should be from other patches on my stack).
comment:11 Changed 12 years ago by
I incline to Jason's argument. I propose 'trace_through' as a more palpable name than Jason's 'trace_execution'.
comment:12 Changed 12 years ago by
Authors:  → William Stein, Jason Grout 

Reviewers:  → Robert Miller 
Status:  needs_review → positive_review 
LGTM.
comment:13 Changed 12 years ago by
Milestone:  sage4.6.1 → sagefeature 

Sorry, but the discussion at sagedevel yielded very few people in favour of this patch, so I'm not planning to merge it.
comment:14 Changed 12 years ago by
Thanks for asking on sagedevel. I'm still in favor of it, since (as a linear algebra person and a math teacher) I'd rather have trace have mathematical meaning instead of programming meaning, and it is a natural complement to having a toplevel det function. My vote wasn't counted on sagedevel yet, so add another +1.
comment:15 Changed 12 years ago by
Milestone:  sagefeature → sagewait 

comment:16 Changed 10 years ago by
Dependencies:  → #13109 

Status:  positive_review → needs_work 
comment:17 Changed 10 years ago by
Reviewers:  Robert Miller → Robert Miller, Volker Braun 

Status:  needs_work → positive_review 
Since there seems to have been a vote that this patch should not be merged, I think we should close it as wontfix.
comment:18 Changed 10 years ago by
Status:  positive_review → needs_info 

I think we should have another vote. I was the major 1 vote, and I fixed the issues I had with it in my updated patch.
I'll post to sagedevel one more time. It's been a long time since this issue was raised.
comment:19 Changed 10 years ago by
I've posted to sagedevel again: https://groups.google.com/group/sagedevel/browse_thread/thread/61ca252973c4930f
comment:20 Changed 7 years ago by
Authors:  William Stein, Jason Grout → William Stein, Jason Grout, Kwankyu Lee 

Branch:  → u/klee/9704 
Commit:  → 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b 
comment:21 Changed 7 years ago by
Commit:  950f0f53b4dd013cf69b544ad31f1c8a8c6e538b → 133b3282fd7cff37ffe159166232d8bd1bd666be 

Branch pushed to git repo; I updated commit sha1. New commits:
133b328  Add tr() instead of trace() for matrices

comment:22 Changed 7 years ago by
As not everyone is happy with refactoring trace()
for dual purposes, we may add tr()
for mathematical trace instead of trace()
. Actually I guess that it is more frequent and convenient to think of tr(m)
than trace(m)
for a nonprogrammer mathematician. Also it nicely balances with det(m)
comment:23 Changed 7 years ago by
Status:  needs_info → needs_review 

comment:24 Changed 7 years ago by
Milestone:  sagepending → sage7.2 

comment:25 Changed 6 years ago by
Description:  modified (diff) 

Status:  needs_review → needs_work 
comment:26 Changed 6 years ago by
Commit:  133b3282fd7cff37ffe159166232d8bd1bd666be → fb6bf0c4695e8593094d5dc8df2c104f056bd708 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fb6bf0c  Add tr() instead of trace() for matrices

comment:27 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:28 Changed 5 years ago by
Commit:  fb6bf0c4695e8593094d5dc8df2c104f056bd708 → b27ee4178439e0345bc21fc37b8353b5b83fac47 

comment:29 Changed 5 years ago by
Dependencies:  #13109 

Description:  modified (diff) 
Milestone:  sage7.2 → sage8.1 
Previously I somehow ended up kidnapping this ticket, and uploaded a patch not for the original purpose of the ticket. I felt guilty, so now I recover the ticket for the original purpose.
The newly pushed branch only contains the code by Jason with slight changes. The code by Jason implements the idea discussed in https://groups.google.com/group/sagedevel/browse_thread/thread/61ca252973c4930f
comment:30 Changed 5 years ago by
Milestone:  sage8.1 → sage8.2 

Status:  needs_review → needs_work 
does not apply
comment:31 Changed 5 years ago by
Commit:  b27ee4178439e0345bc21fc37b8353b5b83fac47 → 55edfa865f08bd173905dae865fae6cd4e3e2aa2 

Branch pushed to git repo; I updated commit sha1. New commits:
55edfa8  Merge develop

comment:32 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:33 Changed 4 years ago by
Milestone:  sage8.2 → sageduplicate/invalid/wontfix 

It seems that there is no enough consensus in changing the meaning of trace
command, which is now commonly used to trace code execution in Sage.
At least I myself prefer to use the methods m.det()
and m.trace()
than the functions det(m)
or trace(m)
. So I would rather remove the function det
instead of introducing new trace
function.
Hence I close this ticket as "won't fix".
1 on this idea. This means that "trace" means *completely* different things depending on the input. Instead, we ought to decide what "trace" means and make two functions. I think trace(x) should be x.trace (and give a mathematical meaning), and the current trace should be code_trace or trace_execution or something like that.