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: 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) Commit: 55edfa865f08bd173905dae865fae6cd4e3e2aa2
Dependencies: Stopgaps:

Description (last modified by klee)

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 was 10 years ago.
trac-9704-trace.patch (4.4 KB) - added by jason 10 years ago.
apply instead of previous patch

Download all attachments as: .zip

Change History (35)

Changed 10 years ago by was

comment:1 Changed 10 years ago by was

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jason

-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 10 years ago by mhansen

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 follow-up: Changed 10 years ago by was

  • 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 jason

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 follow-up: Changed 10 years ago by 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 10 years ago by jason

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 follow-up: Changed 10 years ago by was

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 jason

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 jason

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 10 years ago by jason

apply instead of previous patch

comment:11 Changed 10 years ago by klee

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 rlm

  • Authors set to William Stein, Jason Grout
  • Reviewers set to Robert Miller
  • Status changed from needs_review to positive_review

LGTM.

comment:13 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-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 9 years ago by jason

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 9 years ago by jdemeyer

  • Milestone changed from sage-feature to sage-wait

comment:16 Changed 8 years ago by vbraun

  • Dependencies set to #13109
  • Status changed from positive_review to needs_work

comment:17 Changed 8 years ago by vbraun

  • 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 jason

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

comment:20 Changed 4 years ago by klee

  • Authors changed from William Stein, Jason Grout to William Stein, Jason Grout, Kwankyu Lee
  • Branch set to u/klee/9704
  • Commit set to 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b

comment:21 Changed 4 years ago by git

  • Commit changed from 950f0f53b4dd013cf69b544ad31f1c8a8c6e538b to 133b3282fd7cff37ffe159166232d8bd1bd666be

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

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

comment:22 Changed 4 years ago by klee

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 4 years ago by klee

  • Status changed from needs_info to needs_review

comment:24 Changed 4 years ago by klee

  • Milestone changed from sage-pending to sage-7.2

comment:25 Changed 3 years ago by klee

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:26 Changed 3 years ago by git

  • Commit changed from 133b3282fd7cff37ffe159166232d8bd1bd666be to fb6bf0c4695e8593094d5dc8df2c104f056bd708

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 3 years ago by klee

  • Status changed from needs_work to needs_review

comment:28 Changed 3 years ago by git

  • Commit changed from fb6bf0c4695e8593094d5dc8df2c104f056bd708 to b27ee4178439e0345bc21fc37b8353b5b83fac47

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 3 years ago by klee

  • Dependencies #13109 deleted
  • Description modified (diff)
  • Milestone changed from sage-7.2 to sage-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 2 years ago by chapoton

  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from needs_review to needs_work

does not apply

comment:31 Changed 2 years ago by git

  • Commit changed from b27ee4178439e0345bc21fc37b8353b5b83fac47 to 55edfa865f08bd173905dae865fae6cd4e3e2aa2

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

55edfa8Merge develop

comment:32 Changed 2 years ago by klee

  • Status changed from needs_work to needs_review

comment:33 Changed 11 months ago by klee

  • Milestone changed from sage-8.2 to sage-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.