Opened 10 years ago
Last modified 11 months ago
#9704 needs_review enhancement
refactor "trace"  make trace command call trace method on input if available
Reported by:  was  Owned by:  jason 

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)  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 10 years ago by
comment:1 Changed 10 years ago by
 Status changed from new to needs_review
comment:2 Changed 10 years ago by
comment:3 Changed 10 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 10 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 in reply to: ↑ 4 Changed 10 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 10 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 in reply to: ↑ 6 Changed 10 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 10 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 in reply to: ↑ 8 Changed 10 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 10 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 10 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 9 years ago by
 Reviewers set to Robert Miller
 Status changed from needs_review to positive_review
LGTM.
comment:13 Changed 9 years ago by
 Milestone changed from sage4.6.1 to 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 9 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 9 years ago by
 Milestone changed from sagefeature to sagewait
comment:16 Changed 8 years ago by
 Dependencies set to #13109
 Status changed from positive_review to needs_work
comment:17 Changed 8 years ago by
 Reviewers changed from Robert Miller to Robert Miller, Volker Braun
 Status changed from needs_work to 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 8 years ago by
 Status changed from positive_review to 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 8 years ago by
I've posted to sagedevel again: https://groups.google.com/group/sagedevel/browse_thread/thread/61ca252973c4930f
comment:20 Changed 4 years ago by
 Branch set to u/klee/9704
 Commit set to 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b
comment:21 Changed 4 years ago by
 Commit changed from 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b to 133b3282fd7cff37ffe159166232d8bd1bd666be
Branch pushed to git repo; I updated commit sha1. New commits:
133b328  Add tr() instead of trace() for matrices

comment:22 Changed 4 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 4 years ago by
 Status changed from needs_info to needs_review
comment:24 Changed 4 years ago by
 Milestone changed from sagepending to sage7.2
comment:25 Changed 3 years ago by
 Description modified (diff)
 Status changed from needs_review to needs_work
comment:26 Changed 3 years ago by
 Commit changed from 133b3282fd7cff37ffe159166232d8bd1bd666be to 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 3 years ago by
 Status changed from needs_work to needs_review
comment:28 Changed 3 years ago by
 Commit changed from fb6bf0c4695e8593094d5dc8df2c104f056bd708 to b27ee4178439e0345bc21fc37b8353b5b83fac47
comment:29 Changed 3 years ago by
 Dependencies #13109 deleted
 Description modified (diff)
 Milestone changed from sage7.2 to 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 2 years ago by
 Milestone changed from sage8.1 to sage8.2
 Status changed from needs_review to needs_work
does not apply
comment:31 Changed 2 years ago by
 Commit changed from b27ee4178439e0345bc21fc37b8353b5b83fac47 to 55edfa865f08bd173905dae865fae6cd4e3e2aa2
Branch pushed to git repo; I updated commit sha1. New commits:
55edfa8  Merge develop

comment:32 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:33 Changed 11 months ago by
 Milestone changed from sage8.2 to 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.