Opened 10 years ago

Closed 10 years ago

#11381 closed enhancement (fixed)

Add more simplification methods to symbolic vectors.

Reported by: robertwb Owned by: burcin
Priority: major Milestone: sage-4.7.1
Component: symbolics Keywords:
Cc: Merged in: sage-4.7.1.alpha3
Authors: Robert Bradshaw Reviewers: Joris Vankerschaver
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11335 Stopgaps:

Status badges

Description

Followup to #11335.

Attachments (3)

11381-vector-simplify.patch (3.3 KB) - added by robertwb 10 years ago.
11381-vector-simplify-reviewer-changes.patch (3.8 KB) - added by jvkersch 10 years ago.
11381-vector-simplify-reviewer-changes.2.patch (3.8 KB) - added by robertwb 10 years ago.
apply only this patch

Download all attachments as: .zip

Change History (11)

Changed 10 years ago by robertwb

comment:1 Changed 10 years ago by robertwb

  • Dependencies set to #11335
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jvkersch

  • Reviewers set to Joris Vankerschaver

Nice solution! I'm ready to give this a positive review, but is there a reason why you didn't replace simplify_full by an element-wise method as well?

comment:3 Changed 10 years ago by robertwb

The only reason was that didn't want to trash the custom docstring.

Changed 10 years ago by jvkersch

comment:4 Changed 10 years ago by jvkersch

Hi Robert,

Good point, but I would still advocate replacing simplify_full by an elementwise method: not only does it restore the symmetry between simplify_full and the other elementwise methods, your code is also more robust and generic. And since the docstring for apply already has a test for simplify_full, the changes are only minimal.

I've gone ahead and made this change to your patch. If you agree with the changes, I'm ready to give this a positive review (not sure it's OK to do that, but the changes are very minimal). If not, please delete my patch and I'll approve your original version instead.

comment:5 Changed 10 years ago by robertwb

Your change looks good, though I noticed that the patch didn't have a good description (which was propagated to your update). Fixed. (Yes, it's totally kosher for the original author to +1 the referee's changes.)

Changed 10 years ago by robertwb

apply only this patch

comment:6 Changed 10 years ago by jvkersch

  • Status changed from needs_review to positive_review

Great! Thanks for noticing the absence of a good description, I always forget that.

comment:7 Changed 10 years ago by jdemeyer

  • Authors set to Robert Bradshaw

comment:8 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.