Opened 5 years ago

Closed 5 years ago

#20500 closed enhancement (fixed)

LPAbstractDictionary: Refactor leaving_coefficients, entering_coefficients using new methods row_coefficients, column_coefficients

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-7.2
Component: numerical Keywords: lp
Cc: novoselt Merged in:
Authors: Peijun Xiao, Matthias Koeppe Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: a382ec1 (Commits, GitHub, GitLab) Commit: a382ec183561102e10c979a0f47d90b3f7625d59
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

This is preparation for the cutting plane method (#18805 ), which needs to read out a row of the dictionary. (Seems cleaner than having to do "leave(i), get leaving_coefficients(), leave(None)".)

The patch is also adding @abstract_method stubs for the methods that the concrete dictionaries need to implement.

Change History (11)

comment:1 Changed 5 years ago by mkoeppe

  • Branch set to u/mkoeppe/lpabstractdictionary__refactor_leaving_coefficients__entering_coefficients_using_new_methods_row_coefficients__column_coefficients

comment:2 Changed 5 years ago by mkoeppe

  • Authors set to Peijun Xiao, Matthias Koeppe
  • Cc novoselt added
  • Commit set to d6ee048f324cc6aaaff2168c75ca9e3e6b7e337e
  • Description modified (diff)

New commits:

​d6ee048Refactor leaving_coefficients in terms of new method row_coefficients

comment:3 Changed 5 years ago by git

  • Commit changed from d6ee048f324cc6aaaff2168c75ca9e3e6b7e337e to 4e9955ec46e63490c8c884b9ca74111b42282fdb

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

​3cbdeefFixup
​db52d0eAdd @abstract_method methods to LPAbstractDictionary
​4e9955eRefactor entering_coefficients in terms of new method column_coefficients

comment:4 Changed 5 years ago by mkoeppe

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

comment:5 follow-up: Changed 5 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev
  • Status changed from needs_review to needs_work

I am not convinced there is much point in abstract methods, which leads to duplication of examples and tests, but in any way:

  • leaving_coefficients has wrong documentation
  • column_coefficients has wrong documentation twice

comment:6 Changed 5 years ago by git

  • Commit changed from 4e9955ec46e63490c8c884b9ca74111b42282fdb to a382ec183561102e10c979a0f47d90b3f7625d59

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

​a382ec1Fix documentation

comment:7 Changed 5 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:8 in reply to: ↑ 5 Changed 5 years ago by mkoeppe

Replying to novoselt:

I am not convinced there is much point in abstract methods, which leads to duplication of examples and tests,

I think it's quite nice in combination with _test_not_implemented_methods and as a documentation of the interface.

but in any way:

  • leaving_coefficients has wrong documentation
  • column_coefficients has wrong documentation twice

Fixed.

comment:9 Changed 5 years ago by novoselt

  • Status changed from needs_review to positive_review

It would be nice if documentation of abstract methods somehow "propagated" to concrete implementations so that there were no copy-pasted duplication. But that's of course orthogonal to this ticket.

comment:10 Changed 5 years ago by mkoeppe

I suppose one could copy __doc__ from the abstract method to the concrete one, but I'm not sure whether the patchbot that watches over code coverage would be happy.

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/mkoeppe/lpabstractdictionary__refactor_leaving_coefficients__entering_coefficients_using_new_methods_row_coefficients__column_coefficients to a382ec183561102e10c979a0f47d90b3f7625d59
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.