[Review] Polynomial library review begins today
The review of Pawel Kieliszczyk's Polynomial library begins today and ends on Thurs 19th March. Download of the zip file from the vault is here: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=polynomial.zip&directory=&PHPSESSID=bbc9a84b382be1fc412254cfe30b925b Otherwise the library is present in the sandbox here: https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/ And the docs can be read online here: https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index.... The polynomial library contains a single class - polynomial<FieldType> - used for the manipulation of polynomials, along with a selection of algorithms which operate upon them. The library is an extension/rewrite of the existing "implementation detail" polynomial class in Boost.Math, and was written as part of last years Google Summer of Code under the mentorship of Fernando Cacciola. What to include in Review Comments ~~~~~~~~~~~~~~~~~~~~~~~~~ Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers. The goal of a Boost library review is to improve the library through constructive criticism, and at the end a decision must be made: is the library good enough at this point to accept into Boost? If not, we hope to have provided enough constructive criticism for it to be improved and accepted at a later time. The Serialization library is a good example of how constructive criticism resulted in revisions resulting in an excellent library that was accepted in its second review. Here are some questions you might want to answer in your review: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Many reviews include questions for library authors. Authors are interested in defending their library against your criticisms; otherwise they would not have brought their library up for review. If you don't get a response to your question quickly, be patient; if it takes too long or you don't get an answer you feel is sufficient, ask again or try to rephrase the question. Do remember that English is not the native language for many Boosters, and that can cause misunderstandings. E-mail is a poor communication medium, and even if messages rarely get lost in transmission, they often get drowned in the deluge of other messages. Don't assume that an unanswered message means you're being ignored. Given constructively, criticism will be taken better and have more positive effects, and you'll get the answers you want. John Maddock. Review Manager for Polynomial Library.
Looking briefly, I have 2 comments: 1) The first text is 'Background'. We need some introduction here. An overview of the purpose of the library. 2) """ Modification from std::vector: polynomial<FieldType>& operator=(std::vector<FieldType>& c); This function uses the nested std::vector::swap() function. The c vector should contain new coefficients. """ Does this mean that the vector 'c' is modified? I can't accept this. This really violates the principle of least surprise. Noone expects an operator= to act like that.
After review, I can't find any support for polynomials over finite fields. I would argue that no polynomials library is complete without this functionality built-in. At the very least, one should be able to specify an irreducible polynomial in a finite field and use that as the basis for another field. Better would be a way to know if some polynomial is irreducible over its field. This library doesn't help me much without that functionality. I think the library needs this functionality very badly. I think the interface is fine, few complaints. Maybe allow other containers, and make partially specialized templates for vectors if available on a compiler? Documentation needs proofreading and some fixing, there's some grammatical issues. I haven't actually compiled it though. I read the code and documentation. Should it be in boost? Only if someone adds the requested functionality at some point. Ross Levine
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 10 March 2009, Neal Becker wrote:
2) """ Modification from std::vector: polynomial<FieldType>& operator=(std::vector<FieldType>& c);
This function uses the nested std::vector::swap() function. The c vector should contain new coefficients. """
Does this mean that the vector 'c' is modified? I can't accept this. This really violates the principle of least surprise. Noone expects an operator= to act like that.
I agree with Neal. By the way, there is also a copy constructor with the same problem. Perhaps the operator= overload for std::vector should be renamed to something like "move_from" until a move emulation library gets added into boost? Or, only real moves using rvalue references should be supported. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkm2ix8ACgkQ5vihyNWuA4WsfwCg1caMvHYK+CKngKcRMZv8JMzH G/QAnAjAPW4Wr9cs4QNoMI+fdDNB97ad =7ycd -----END PGP SIGNATURE-----
Yes, moving from lvalues with copy or assignment syntax is never a good idea. Sent from my iPhone On Mar 10, 2009, at 8:45 AM, Frank Mori Hess <frank.hess@nist.gov> wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Tuesday 10 March 2009, Neal Becker wrote:
2) """ Modification from std::vector: polynomial<FieldType>& operator=(std::vector<FieldType>& c);
This function uses the nested std::vector::swap() function. The c vector should contain new coefficients. """
Does this mean that the vector 'c' is modified? I can't accept this. This really violates the principle of least surprise. Noone expects an operator= to act like that.
I agree with Neal. By the way, there is also a copy constructor with the same problem. Perhaps the operator= overload for std::vector should be renamed to something like "move_from" until a move emulation library gets added into boost? Or, only real moves using rvalue references should be supported. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAkm2ix8ACgkQ5vihyNWuA4WsfwCg1caMvHYK+CKngKcRMZv8JMzH G/QAnAjAPW4Wr9cs4QNoMI+fdDNB97ad =7ycd -----END PGP SIGNATURE----- _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Tuesday 10 March 2009 08:23:07 John Maddock wrote:
Download of the zip file from the vault is here:
Is this the latest version? It does not compile for me, since polynomial.hpp uses pow on lines 370 and 372. It needs to include cmath and also needs a "using std::pow;" statement. Second, there are compile errors upon trying to multiply two polynomials together (code attached). I am using boost 1.38.0 with gcc 4.3.2 on Linux x86_64. The errors I get are of the following form: /usr/lib/gcc/x86_64-redhat- linux/4.3.2/../../../../include/c++/4.3.2/bits/stl_algo.h: In function ‘_OIter std::transform(_IIter, _IIter, _OIter, _UnaryOperation) [with _IIter = __gnu_cxx::__normal_iterator<FieldElem*, std::vector<FieldElem, std::allocator<FieldElem> > >, _OIter = std::back_insert_iterator<std::vector<std::complex<double>, std::allocator<std::complex<double> > > >, _UnaryOperation = std::pointer_to_unary_function<double, FieldElem>]’: ./boost/polynomial.hpp:378: instantiated from ‘boost::math::tools::polynomial<IntegerType>& boost::math::tools::polynomial<FieldType>::operator*=(const boost::math::tools::polynomial<U>&) [with U = FieldElem, FieldType = FieldElem]’ polytest.cc:130: instantiated from here /usr/lib/gcc/x86_64-redhat- linux/4.3.2/../../../../include/c++/4.3.2/bits/stl_algo.h:4281: error: no match for call to ‘(std::pointer_to_unary_function<double, FieldElem>) (FieldElem&)’ /usr/lib/gcc/x86_64-redhat- linux/4.3.2/../../../../include/c++/4.3.2/bits/stl_function.h:424: note: candidates are: _Result std::pointer_to_unary_function<_Arg, _Result>::operator()(_Arg) const [with _Arg = double, _Result = FieldElem] Regards, Ravi
John Maddock wrote:
The review of Pawel Kieliszczyk's Polynomial library begins today and ends on Thurs 19th March.
This library seems to fill a fairly useful niche. This isn't a review, but may turn in to one .... 1) Others have mentioned this, but I want to reiterate: the currently documented behavior of the operator= and copy constructors that take (and modify!) std::vectors are enough by themselves to a make me vote against acceptance. I expect, however, that these will be fixed. Actually, I don't see a need for either of them, as there are constructors taking InputIterators; they're much more general, of course, as they can construct a polynomial from any other type of sequence. 2) evaluate_polynomial() and evaluate_polynomial_faithfully() take their coefficient arguments in the opposite order of all the other functions/constructors in the library, without providing any reason for that choice. That's seems somewhat surprising. I'd suggest fixing that, or at the very least providing an argument in the documentation why this makes some sense. 3) The documentation is a bit thin, and needs a good proofreading a) From the examples are nothing that a competent programmer couldn't figure out from the declarations. Some more interesting or useful examples would be nice, particularly for things like the special forms. It's not exactly clear what I would do with those functions. b) There is no documentation or references to the various algorithms used. Those, too, would be nice. -- ------------------------------------------------------------------------------- Kevin Lynch voice: (617) 353-6025 Physics Department Fax: (617) 353-9393 Boston University office: PRB-361 590 Commonwealth Ave. e-mail: krlynch@bu.edu Boston, MA 02215 USA http://mulan.bu.edu/~krlynch -------------------------------------------------------------------------------
Hi everyone, I'll try to to answer for a few question that have already appeared and explain some implementation issues. Neal Becker wrote:
1) The first text is 'Background'. We need some introduction here. An
overview of the purpose of the library.
I guess you're thinking of docs. I'll try to add the introduction soon.
2) """
Modification from std::vector:
polynomial<FieldType>& operator=(std::vector<FieldType>& c);
This function uses the nested std::vector::swap() function. The c vector
should contain new coefficients.
"""
Does this mean that the vector 'c' is modified? I can't accept this. This
really violates the principle of least surprise. Noone expects an operator=
to act like that.
The reason I decided to add this was performance. Using std::vector::swap() function works faster than copying all coefficients. It might be useful while creating polynomials. We could prepare the coefficients in std::vector() and then simply swap it. On the other hand I agree that using it in operator= is not a good choice. Do you thing that such a function that uses std::vector::swap() should exist in the lib? Actually it doesn't have to be supported but might be. Ross Levine wrote:
After review, I can't find any support for polynomials over finite
fields. I would argue that no polynomials library is complete without
this functionality built-in. At the very least, one should be able to
specify an irreducible polynomial in a finite field and use that as
the basis for another field. Better would be a way to know if some
polynomial is irreducible over its field. This library doesn't help me
much without that functionality.
I think the library needs this functionality very badly.
True. There is no support for this. However, I am not sure I could add this functionality during the review. I think I would need more time and reflections.
Documentation needs proofreading and some fixing, there's some
grammatical issues.
Yea, I realize that my english isn't good but I try my best. Ravi wrote:
On Tuesday 10 March 2009 08:23:07 John Maddock wrote:
Download of the zip file from the vault is here:
Is this the latest version? It does not compile for me, since
polynomial.hpp
uses pow on lines 370 and 372. It needs to include cmath and also needs a
"using std::pow;" statement.
Second, there are compile errors upon trying to multiply two polynomials
together (code attached). I am using boost 1.38.0 with gcc 4.3.2 on Linux
x86_64.
Hm... I've been writing the library on Linux platform using Boost 1.36.0. As far as I know my mentor Fernando Cacciola was using the lib on Windows and he hasn't notified me about any problems. Kevin Lynch wrote:
2) evaluate_polynomial() and evaluate_polynomial_faithfully() take their
coefficient arguments in the opposite order of all the other
functions/constructors in the library, without providing any reason for that
choice. That's seems somewhat surprising. I'd suggest fixing that, or at the
very least providing an argument in the documentation why this makes some
sense.
Yea. We've been thinking about this problem with Fernando. I'll paste here the justification I send to him once:
Evaluation may be implemented in 4 ways:
i) same as now: *first is the coefficient for x^n term and *(last-1) is the
coefficient for x^0 term. Implementation is very simple and fast, then. ii) using bidirectional iterator. Thus, *first would be the coefficient for x^0
term, but operator-- should be provided. iii) using sperator[] like in the rational.hpp from boost libraries
template <class T, class U> inline U evaluate_polynomial(const T* poly, U const& z, std::size_t count) { BOOST_ASSERT(count > 0); U sum = static_cast<U>(poly[count - 1]); for(int i = static_cast<int>(count) - 2; i >= 0; --i) { sum *= z; sum += static_cast<U>(poly[i]); } return sum; }
iv) We could also copy and reverse all coefficients if *first is the coefficient
for x^0 term, then evaluate.
I don't know which way would be the best. Do you have any other ideas?
After a short discussion we decided to implement it the way you can see. -- Pawel Kieliszczyk
Hi Paweł, On Wednesday 11 March 2009 06:47:28 Paweł Kieliszczyk wrote:
Is this the latest version? It does not compile for me, since polynomial.hpp
uses pow on lines 370 and 372. It needs to include cmath and also needs a "using std::pow;" statement.
Even if <cmath> is included indirectly somewhere else (in your version of boost), you should still explicitly include it in polynomial.hpp since it is a direct dependency and since the external header that includes <cmath> may change in a future revision.
Second, there are compile errors upon trying to multiply two polynomials together (code attached). I am using boost 1.38.0 with gcc 4.3.2 on Linux x86_64.
Hm... I've been writing the library on Linux platform using Boost 1.36.0. As far as I know my mentor Fernando Cacciola was using the lib on Windows and he hasn't notified me about any problems.
Did you try compiling the test case attached in my previous email? Note that the compilation errors are on Linux, not Windows. It is possible that some requirement for FieldType is not satisfied by my FieldElem class which implements GF(256) arithmetic. However, the documentation is completely silent on the requirements for FieldType (as Paul Bristow pointed out). Please let me know if any requirements are not met. Regards, Ravi PS: Please reply to this mail directly; combining replies to several threads in a single email makes it hard to follow separate sub-threads of a discussion.
I haven't been able to evaluate this properly, but here were some quick thoughts: - Rename evaluate() to horner() and provide evaluate() as an inline call to horner(). - Rename evaluate_faithfully() to compensated_horner(). - Things that say "Be careful about this" generally need fixing. - It would be nice to have operator* and operator/ as well as *= and /=. - Considering they share the same core algorithm, it might be nice to have a function that could do division and return the remainder at the same time. Something like poly::divide(P &divisor, P "ient, P &remainer) - FFTs are good for multiplying polynomials above a certain size, but they are rather heavy for small polynomials and can lead to unexpected crosstalk between terms. There should be an option for * that doesn't use the FFT, and it should be the default for small polynomials. - operator/= has a hardcoded "std::vector<double> new_coefficients" that later gets rounded?? Maybe that works. Later, Daniel
I've not been able look at this much and will try to do better review if time permits, but here's a very small piece of feedback: https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index.... - typo "substraction" should be "subtraction". https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index.... - typo "Greates Common" should be "Greatest Common". This _extremely_ basic program failed to compile under MSVC 2003 SP1 (7.1): #include <boost/polynomial.hpp> int main() { } with these errors: c:\polynomial\boost\polynomial.hpp(109) : error C2783: 'boost::math::tools::polynomial<IntegerType> boost::math::tools::gcd(boost::math::tools::polynomial<IntegerType>,boost::math::tools::polynomial<IntegerType>)' : could not deduce template argument for 'IntegerType' c:\polynomial\boost\polynomial.hpp(33) : see declaration of 'boost::math::tools::gcd' c:\polynomial\boost\polynomial.hpp(248) : see reference to class template instantiation 'boost::math::tools::polynomial<FieldType>' being compiled c:\polynomial\boost\polynomial.hpp(109) : error C2063: 'boost::math::tools::gcd' : not a function I am using Boost 1.38.0. It stopped me looking further at the library. I'll try to get past this and continue if time permits... Regards, Peter Barker
On Thu, Mar 12, 2009 at 12:55 PM, Peter Barker <newbarker@gmail.com> wrote:
I've not been able look at this much and will try to do better review if time permits, but here's a very small piece of feedback:
https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index.... - typo "substraction" should be "subtraction".
https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index.... - typo "Greates Common" should be "Greatest Common".
This _extremely_ basic program failed to compile under MSVC 2003 SP1 (7.1):
#include <boost/polynomial.hpp>
int main() { }
with these errors:
c:\polynomial\boost\polynomial.hpp(109) : error C2783: 'boost::math::tools::polynomial<IntegerType> boost::math::tools::gcd(boost::math::tools::polynomial<IntegerType>,boost::math::tools::polynomial<IntegerType>)' : could not deduce template argument for 'IntegerType' c:\polynomial\boost\polynomial.hpp(33) : see declaration of 'boost::math::tools::gcd' c:\polynomial\boost\polynomial.hpp(248) : see reference to class template instantiation 'boost::math::tools::polynomial<FieldType>' being compiled c:\polynomial\boost\polynomial.hpp(109) : error C2063: 'boost::math::tools::gcd' : not a function
I am using Boost 1.38.0. It stopped me looking further at the library. I'll try to get past this and continue if time permits...
Failed with the same error on MSVC 2008 Express Edition. Can the library author tell me what's going wrong and how I can get past this so I can test the library out please? Regards, Peter Barker
-----Original Message----- From: boost-bounces@lists.boost.org [mailto:boost-bounces@lists.boost.org] On Behalf Of John Maddock Sent: 10 March 2009 12:23 To: boost@lists.boost.org; boost-users@lists.boost.org Subject: [boost] [Review] Polynomial library review begins today
The review of Pawel Kieliszczyk's Polynomial library begins today and ends on Thurs 19th March.
Here are some questions you might want to answer in your review:
* What is your evaluation of the design? * What is your evaluation of the implementation?
Many have already commented on modifying the vector being too surprising - I agree - but is the speed a real issue?
* What is your evaluation of the documentation?
OK as far as it goes but won't sell it to those who are not yet sure they ought to be using this library. And I thought it needs: much more details and comments. references and links to some general texts. reference to algorithms used. Links to the worked examples - and they need some comments and sample output too. Some editorial work by someone other than the author. Needs to mention and link to tests (they appear to exist and do something that I am not qualified to judge effectiveness). No tests using big int - and no clues about when the 32 bit int will 'run out of steam'. For those whose math didn't penetrate as far Galois, an explanation of use of FieldType template parameter is important. Doc.html appear to have been created 'the Hard Way'. Would be much more useful and look nicer if produced with the Quickbook, Doxygen... toolchain. And make it maintainable by other people. As a simple example, including the actual text of the Synposis makes it appear in full glorious Technicolor - and ensures that it remains up to date too.
* What is your evaluation of the potential usefulness of the library?
A bit of a niche application, but important for those who need it. Also shows that we badly need a Big Integer in Boost. (and a Big Floating-Point too).
* Did you try to use the library? With what compiler? Did you have any
problems? Started to try a demo with VS 2008 Sp1 #include <boost/polynomial.hpp> int main() { boost::math::tools::polynomial<int> poly1; // zero } But failed I:\boost-sandbox\SOC\2008\polynomial\boost/polynomial.hpp(120) : error C2783: 'boost::math::tools::polynomial<IntegerType> boost::math::tools::gcd(boost::math::tools::polynomial<IntegerType>,boost::m ath::tools::polynomial<IntegerType>)' : could not deduce template argument for 'IntegerType' I:\boost-sandbox\SOC\2008\polynomial\boost/polynomial.hpp(33) : see declaration of 'boost::math::tools::gcd' I:\boost-sandbox\SOC\2008\polynomial\boost/polynomial.hpp(248) : see reference to class template instantiation 'boost::math::tools::polynomial<IntegerType>' being compiled But ran out of time.
* How much effort did you put into your evaluation?
A glance at the documentation.
* Are you knowledgeable about the problem domain?
No
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
With quite a few improvements, yes. Paul --- Paul A. Bristow Prizet Farmhouse Kendal, UK LA8 8AB +44 1539 561830, mobile +44 7714330204 pbristow@hetp.u-net.com
Just a reminder that the review of the polynomial lib ends on Thursday, the original announcement is below. John Maddock (review manager)
The review of Pawel Kieliszczyk's Polynomial library begins today and ends on Thurs 19th March.
Download of the zip file from the vault is here: http://www.boostpro.com/vault/index.php?action=downloadfile&filename=polynomial.zip&directory=&PHPSESSID=bbc9a84b382be1fc412254cfe30b925b
Otherwise the library is present in the sandbox here: https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/
And the docs can be read online here: https://svn.boost.org/svn/boost/sandbox/SOC/2008/polynomial/libs/docs/index....
The polynomial library contains a single class - polynomial<FieldType> - used for the manipulation of polynomials, along with a selection of algorithms which operate upon them. The library is an extension/rewrite of the existing "implementation detail" polynomial class in Boost.Math, and was written as part of last years Google Summer of Code under the mentorship of Fernando Cacciola.
What to include in Review Comments ~~~~~~~~~~~~~~~~~~~~~~~~~
Your comments may be brief or lengthy, but basically the Review Manager needs your evaluation of the library. If you identify problems along the way, please note if they are minor, serious, or showstoppers.
The goal of a Boost library review is to improve the library through constructive criticism, and at the end a decision must be made: is the library good enough at this point to accept into Boost? If not, we hope to have provided enough constructive criticism for it to be improved and accepted at a later time. The Serialization library is a good example of how constructive criticism resulted in revisions resulting in an excellent library that was accepted in its second review.
Here are some questions you might want to answer in your review:
* What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain?
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
Many reviews include questions for library authors. Authors are interested in defending their library against your criticisms; otherwise they would not have brought their library up for review. If you don't get a response to your question quickly, be patient; if it takes too long or you don't get an answer you feel is sufficient, ask again or try to rephrase the question. Do remember that English is not the native language for many Boosters, and that can cause misunderstandings.
E-mail is a poor communication medium, and even if messages rarely get lost in transmission, they often get drowned in the deluge of other messages. Don't assume that an unanswered message means you're being ignored. Given constructively, criticism will be taken better and have more positive effects, and you'll get the answers you want.
John Maddock. Review Manager for Polynomial Library.
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
-------------------------------------------------------------------------------- No virus found in this incoming message. Checked by AVG - www.avg.com Version: 8.0.237 / Virus Database: 270.11.9/1988 - Release Date: 03/06/09 19:17:00
A little later than planned, here is my review of the polynomial library: Documentation and Design ~~~~~~~~~~~~~~~~~~~~~~~~ As previously noted by others the Introduction doesn't adequately describe the library - some sort of simple tutorial at this point would be useful too. The "Background" section doesn't really describe the "background" as such either. This constructor: template<typename U> polynomial(const U& v); had me a little confused at first - is there any advantage to making this a template rather than simply accepting a const FieldType& as the single argument? This constructor: polynomial(std::vector<FieldType>& c); should *not* modify the original vector IMO, a construct-with-move should be a separate constructor - I'm sure we have a move/rvalue reference emulation library available somewhere? Likewise for the assignment operator polynomial<FieldType>& operator=(std::vector<FieldType>& c); It's not clear from the documentation what this constructor does: polynomial(InputIterator first1, InputIterator last1, InputIterator first2); It needs to clearly state the degree of the resulting polynomial and whether it passes through all/any of the points - in fact maybe the constructor should have a final parameter for the degree of the polynomial and use least-squares fitting when required? Likewise for the member function template<typename InputIterator> void interpolate(InputIterator first1, InputIterator last1, InputIterator first2); Operators: the / and % operators need good descriptions of what they do, given that no exact division is possible. As someone else has already noted, a function to calculate divide and remainder in one step would be useful too. GCD: presumably this is restricted to polynomials with integer coefficients? If so it should say so. Evaluation: The docs should say what method is used for the evaluate_faithfully method and give a reference. Renaming as someone else suggested may be better too, but personally I'm easy either way on that. I'm not sure whether there should be an evaluate_by_preconditioning method, shouldn't the polynomial "know" that it has been preconditioned and react accordingly. I haven't looked/checked but do the usual arithmetic operators still work if the polynomial has been preconditioned? I assume probably not, but if that's the case there should be a stern warning to that effect, *and* checks in code to prevent us from doing something stupid :-) BTW, preconditioning can be applied to polynomials of any degree I believe it's just that it gets hard to implement in the generic case. In addition I'd like to see the evaluation functions templated, so that the type being evaluated can differ from FieldType - it would surely be quite common for example to create and manipulate polynomials with integer coefficients, but then want to evaluate on a floating point type. There is machinery in Boost.Math BTW to handle the mixed argument-promotion and calculation of the result type, let me know if you need help in using this. Special Forms: the functions provided do *not* generate polynomials in the alternative forms, but rather generate the named polynomial of order N, which is rather less useful, but at the very least the docs need updating to reflect this. Implementation ~~~~~~~~~~~~~~ I haven't spent a great deal of time on this, but I note that: test_arithmetics.cpp fails on cygwin: or at least it did once I added a using std::pow; to start of operator*= to get things compiling. None of the code builds with msvc: there are a couple of issues here: friend polynomial<FieldType> boost::math::tools::gcd<>(polynomial<FieldType> u, polynomial<FieldType> v); Needs to be changed to: friend polynomial<FieldType> boost::math::tools::gcd<FieldType>(polynomial<FieldType> u, polynomial<FieldType> v); to get msvc to Grok the code. After that there are a bunch of failures due to the use of log2 which msvc doesn't support (and since it's not part of the std other compilers are likely to complain too). I haven't looked too hard at the implementation, except to echo the comments posted previously that the Conceptual requirements for FieldType need documenting and *testing*. There are concept checking classes and archetypes in Boost.Math which may be of use here. Again, shout if you need help in figuring out how to use these. I did notice that operator*= for example uses std::complex<double> internally which effectively limits the precision that can be achieved - to be useful to me - and to replace the existing "implementation detail" polynomial class in Boost.Math - I would need the polynomial class to be usable with arbitrary precision types such as NTL::RR or mpfr_class. Also as previously noted, multiplication of small polynomials may well be more efficient with the "naive" algorithm, so some performance tests and experimenting is required here. Related to this, use of "round" should be restricted to types that are known to be integers? * What is your evaluation of the potential usefulness of the library? As noted by others it's a niche library but potentially quite useful within that niche. * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. I think that the library could be accepted into Boost, but not it's it's current form, so I would vote no for now: please note this is my personal review of the library and should not be confused with my role as review manager for this library. Regards, John Maddock.
participants (11)
-
David Abrahams -
dherring＠ll.mit.edu -
Frank Mori Hess -
John Maddock -
Kevin Lynch -
Neal Becker -
Paul A. Bristow -
Paweł Kieliszczyk -
Peter Barker -
Ravi -
Ross Levine