Opened 3 years ago
Closed 3 years ago
#26356 closed enhancement (fixed)
Minimal approximant bases
Reported by:  vneiger  Owned by:  vneiger 

Priority:  major  Milestone:  sage8.7 
Component:  algebra  Keywords:  polynomial matrix; approximant basis; HermitePadé approximation 
Cc:  ghromainlebreton  Merged in:  
Authors:  Seung Gyu Hyun, Vincent Neiger  Reviewers:  Romain Lebreton, Pascal Giorgi 
Report Upstream:  N/A  Work issues:  
Branch:  fe6472e (Commits, GitHub, GitLab)  Commit:  fe6472ef2b732ad09d82971dfb538d92feaf53b7 
Dependencies:  Stopgaps: 
Description
New functionalities:
 computation of shifted minimal approximant bases (iterative algorithm),
 verification that a matrix is a shifted minimal approximant basis.
This should be done in a general context:
 accepting nonuniform orders and nonuniform shifts,
 allowing to work rowwise or columnwise,
 offering the possibility of obtaining the canonical basis (that is, the one in shifted Popov form).
Change History (19)
comment:1 Changed 3 years ago by
 Branch set to u/vneiger/minimal_approximant_bases
comment:2 Changed 3 years ago by
 Commit set to c25863aa7eab0f003fa19f5095b37eeb79107d28
comment:3 Changed 3 years ago by
The features mentioned in the ticket have been implemented, with complete documentation and tests. After a few more tests, for safety, this will be ready for review.
comment:4 Changed 3 years ago by
 Commit changed from c25863aa7eab0f003fa19f5095b37eeb79107d28 to 00238d638be9f449262e7445e3ab7760ff2d148e
Branch pushed to git repo; I updated commit sha1. New commits:
00238d6  add info for changes on top of file

comment:5 Changed 3 years ago by
 Commit changed from 00238d638be9f449262e7445e3ab7760ff2d148e to b300f4fb2c5eb2cf5f11cf0b82be9fd4af599fe0
Branch pushed to git repo; I updated commit sha1. New commits:
b300f4f  simplification of tests for actual implementation of approx_basis

comment:6 Changed 3 years ago by
 Owner changed from (none) to vneiger
comment:7 Changed 3 years ago by
 Status changed from new to needs_review
Code is ready for review. Thank you for suggestions on improvements.
comment:8 Changed 3 years ago by
 Commit changed from b300f4fb2c5eb2cf5f11cf0b82be9fd4af599fe0 to 667404643b586ab976799a4bb0cadc00df430078
Branch pushed to git repo; I updated commit sha1. New commits:
6674046  minor correction in link to bibliography

comment:9 Changed 3 years ago by
 Branch changed from u/vneiger/minimal_approximant_bases to u/ghromainlebreton/minimal_approximant_bases
comment:10 Changed 3 years ago by
 Commit changed from 667404643b586ab976799a4bb0cadc00df430078 to a6d0b4b4567dc8eda6f3011a3866939544afb71e
Branch pushed to git repo; I updated commit sha1. New commits:
a6d0b4b  Minor mdofications:

comment:11 Changed 3 years ago by
 Commit changed from a6d0b4b4567dc8eda6f3011a3866939544afb71e to 86e5225b64a33e7ce58763ec4071e6d10fe1baba
comment:12 Changed 3 years ago by
Hello vneiger,
your code was great. I have only noticed a few problems that I solved. Could you please double check my changes ? If everything is fine by you and if the continuous integration passes, then you can mark it as a positive review. Best, Romain
comment:13 Changed 3 years ago by
Quick question : why the function names is_minimal_approximant_basis and approximant_basis differ ? Should we rename the 2nd function minimal_approximant_basis ?
comment:14 Changed 3 years ago by
Hi,
Thank you very much for your reviewing and improvements of the code. All this looks good to me now.
I would only suggest to indeed change the name to "minimal_approximant_basis", and to replace "order should be positive integers" with "order should consist of positive integers". May I let you do this? For some reason, with the branch change you made, I have difficulties pushing my changes (I get in detached HEAD for a reason I'm not sure to understand...).
Thank you.
comment:15 Changed 3 years ago by
 Commit changed from 86e5225b64a33e7ce58763ec4071e6d10fe1baba to fe6472ef2b732ad09d82971dfb538d92feaf53b7
Branch pushed to git repo; I updated commit sha1. New commits:
fe6472e  Rename approximant_basis into minimal_approximant_basis

comment:16 Changed 3 years ago by
Done ! LGTM
comment:17 Changed 3 years ago by
 Milestone changed from sage8.4 to sage8.7
 Status changed from needs_review to positive_review
comment:18 Changed 3 years ago by
Thank you for your work!
comment:19 Changed 3 years ago by
 Branch changed from u/ghromainlebreton/minimal_approximant_bases to fe6472ef2b732ad09d82971dfb538d92feaf53b7
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
first version of approximant_basis including normal_form management
first version of is_approximant_basis, just missing generation verification
some cleaning and more examples in is_minimal_approx_basis
cleaning the doc
add some examples for approximant_basis
finished a first working clean version of minimal approximant basis (+ verification)