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: sage-duplicate/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:

Status badges

Description (last modified by Kwankyu Lee)

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)

trac_9704-trace.patch (2.3 KB) - added by William Stein 12 years ago.
trac-9704-trace.patch (4.4 KB) - added by Jason Grout 12 years ago.
apply instead of previous patch

Download all attachments as: .zip

Change History (35)

Changed 12 years ago by William Stein

Attachment: trac_9704-trace.patch added

comment:1 Changed 12 years ago by William Stein

Status: newneeds_review

comment:2 Changed 12 years ago by Jason Grout

-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.

comment:3 Changed 12 years ago by Mike Hansen

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 Changed 12 years ago by William Stein

  • 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 12 years ago by Jason Grout

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 Changed 12 years ago by John 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?

comment:7 in reply to:  6 Changed 12 years ago by Jason Grout

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 non-mathematical, on a completely different level (a programming nuts-and-bolts 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 Changed 12 years ago by William Stein

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 12 years ago by Jason Grout

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 Jason Grout

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).

Changed 12 years ago by Jason Grout

Attachment: trac-9704-trace.patch added

apply instead of previous patch

comment:11 Changed 12 years ago by Kwankyu Lee

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 Robert Miller

Authors: William Stein, Jason Grout
Reviewers: Robert Miller
Status: needs_reviewpositive_review

LGTM.

comment:13 Changed 12 years ago by Jeroen Demeyer

Milestone: sage-4.6.1sage-feature

Sorry, but the discussion at sage-devel yielded very few people in favour of this patch, so I'm not planning to merge it.

comment:14 Changed 12 years ago by Jason Grout

Thanks for asking on sage-devel. 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 top-level det function. My vote wasn't counted on sage-devel yet, so add another +1.

comment:15 Changed 12 years ago by Jeroen Demeyer

Milestone: sage-featuresage-wait

comment:16 Changed 10 years ago by Volker Braun

Dependencies: #13109
Status: positive_reviewneeds_work

comment:17 Changed 10 years ago by Volker Braun

Reviewers: Robert MillerRobert Miller, Volker Braun
Status: needs_workpositive_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 Jason Grout

Status: positive_reviewneeds_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 sage-devel one more time. It's been a long time since this issue was raised.

comment:20 Changed 7 years ago by Kwankyu Lee

Authors: William Stein, Jason GroutWilliam Stein, Jason Grout, Kwankyu Lee
Branch: u/klee/9704
Commit: 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b

comment:21 Changed 7 years ago by git

Commit: 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b133b3282fd7cff37ffe159166232d8bd1bd666be

Branch pushed to git repo; I updated commit sha1. New commits:

133b328Add tr() instead of trace() for matrices

comment:22 Changed 7 years ago by Kwankyu Lee

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 non-programmer mathematician. Also it nicely balances with det(m)

comment:23 Changed 7 years ago by Kwankyu Lee

Status: needs_infoneeds_review

comment:24 Changed 7 years ago by Kwankyu Lee

Milestone: sage-pendingsage-7.2

comment:25 Changed 6 years ago by Kwankyu Lee

Description: modified (diff)
Status: needs_reviewneeds_work

comment:26 Changed 6 years ago by git

Commit: 133b3282fd7cff37ffe159166232d8bd1bd666befb6bf0c4695e8593094d5dc8df2c104f056bd708

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fb6bf0cAdd tr() instead of trace() for matrices

comment:27 Changed 6 years ago by Kwankyu Lee

Status: needs_workneeds_review

comment:28 Changed 5 years ago by git

Commit: fb6bf0c4695e8593094d5dc8df2c104f056bd708b27ee4178439e0345bc21fc37b8353b5b83fac47

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

133b328Add tr() instead of trace() for matrices
22fbba6Merge branch 'develop' into trac9704
b27ee41Trace command call trace method on input

comment:29 Changed 5 years ago by Kwankyu Lee

Dependencies: #13109
Description: modified (diff)
Milestone: sage-7.2sage-8.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/sage-devel/browse_thread/thread/61ca252973c4930f

comment:30 Changed 5 years ago by Frédéric Chapoton

Milestone: sage-8.1sage-8.2
Status: needs_reviewneeds_work

does not apply

comment:31 Changed 5 years ago by git

Commit: b27ee4178439e0345bc21fc37b8353b5b83fac4755edfa865f08bd173905dae865fae6cd4e3e2aa2

Branch pushed to git repo; I updated commit sha1. New commits:

55edfa8Merge develop

comment:32 Changed 5 years ago by Kwankyu Lee

Status: needs_workneeds_review

comment:33 Changed 4 years ago by Kwankyu Lee

Milestone: sage-8.2sage-duplicate/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".

Note: See TracTickets for help on using tickets.