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: |
Description (last modified by )
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
- 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
- Cc novoselt added
- Commit set to d6ee048f324cc6aaaff2168c75ca9e3e6b7e337e
- Description modified (diff)
comment:3 Changed 5 years ago by
- Commit changed from d6ee048f324cc6aaaff2168c75ca9e3e6b7e337e to 4e9955ec46e63490c8c884b9ca74111b42282fdb
comment:4 Changed 5 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:5 follow-up: ↓ 8 Changed 5 years ago by
- 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 documentationcolumn_coefficients
has wrong documentation twice
comment:6 Changed 5 years ago by
- Commit changed from 4e9955ec46e63490c8c884b9ca74111b42282fdb to a382ec183561102e10c979a0f47d90b3f7625d59
Branch pushed to git repo; I updated commit sha1. New commits:
a382ec1 | Fix documentation
|
comment:7 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:8 in reply to: ↑ 5 Changed 5 years ago by
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 documentationcolumn_coefficients
has wrong documentation twice
Fixed.
comment:9 Changed 5 years ago by
- 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
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
- 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
New commits:
Refactor leaving_coefficients in terms of new method row_coefficients