This is not a review, just a few things I've caught while reading the odeint docs. Mostly cosmetic issues, but a couple of deeper questions. * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_... - "They also occur if partial differential equations (PDEs) are discretized in one coordinate."; wouldn't you have to discretize in all but one coordinate? * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_... - I see no mention of any headers except the global one boost/numeric/odeint.hpp. Is it useful or possible to include only parts of the library? * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_... - "a simple function calculating f(x)'"; I think you mean either "f(x)" or "x'", not "f(x)'". * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial... - There's one case here where you use "--" as an em dash. "on the example of explicit_error_rk54_ck -- a 5th order Runge-Kutta method". There are various other places throughout the docs which use just "-" for em dash. It would be best if you could use a proper em dash character, but in any case you should be consistent. * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial... - "which lead to the whole new subject of Chaos Theory"; s/lead/led/ * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial... - "the so called Rosenbrock method"; is "so called" necessary? * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial... - "the first numerical experiment which has been implemented on a computer in 1953. It was studied at Los Alamos on one of the first computer (a MANIAC I) and it triggered a whole new tree of mathematical and physical science."; This is ambiguous (it could mean the first experiment amongst those in 1953). If you really mean it was the first computational numerics experiment then you should probably say something like "the first numerical experiment to be implemented on a computer. It was studied at Los Alamos in 1953 on one of the first computers (a MANIAC I) and triggered a whole new tree of mathematical and physical science." - "For example the unit test for Boost.Units take up to 4 GB of memory at compilation."; are you referring to the unit tests of Boost.Units, or a unit test of odeint that uses Boost.Units? Please clarify. - The matrix-as-state example has a bug: for( size_t j=0 ; j<x.size2() ; ++j ) dxdt( 0 , j ) = dxdt( 0 , x.size1() -1 ) = 0.0; should be for( size_t j=0 ; j<x.size2() ; ++j ) dxdt( 0 , j ) = dxdt( x.size1() -1, j ) = 0.0; This bug makes me think that you ought to provide a debug mode where every entry in dxdt is set to NaN before calling the system operator(), and then they are checked to be non-NaN afterwards. Of course, this would only work for types with a NaN value, but I think all your examples do have that (except maybe the multiprecision types). - "This is in principle all"; awkward phrasing, perhaps "In principle this is all" - I assume you know about the unfinished chunk of docs re PDEs and equations on networks. - "for full support the min( x , y ), max( x , y ), pow( y , y ) must be callable"; what are x and y? Is it significant that pow has both arguments equal but min and max don't? I hope the requirements on the numeric type are laid out more formally elsewhere; please link to that. - "We exemplary show this for a Hamiltonian system"; "exemplary" doesn't work here. You could use s/exemplary show/demonstrate/. * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial... - "simply to small"; s/to/too/ * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i... - "Implicit solvers do only work"; s/do // - "They safe a part of their history" s/safe/save/ - In the section on controlled steppers, the formulae are getting sufficiently complex that it might be worth trying to use TeX-generated pngs rather than straight HTML (as is done in Boost.Math, e.g. <http://www.boost.org/doc/libs/1_51_0/libs/math/doc/sf_and_dist/html/math_toolkit/dist/stat_tut/weg/st_eg/tut_mean_intervals.html>) - I can't figure out what the three lines after table 1.6 are about. - The long line "boost::numeric::odeint::result_of::make_dense_output< r..." in Dense output steppers is too long for my screen; can you break it? - The resizing policy names are a bit clumsy; I think always_resize, initially_resize, and never_resize might be better. - "Typical use cases for this kind of resizer are self expanding lattices like shown in the tutorial"; can you link to the relevant example in the tutorial? - "So it might be suitable that the system function itself is a pair of functions, one for computing the deterministic and one for the stochastic part. The first element of the pair simply computes the deterministic part while the second the stochastic one."; this is rather redundant; how about "So it might be suitable that the system function is a pair of functions. The first element of the pair computes the deterministic part and the second the stochastic one." * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i... - Personally I think the ranges are more useful than the iterators, so it would be nice if they were mentioned in the section title (e.g. "Iterators and Ranges") - "that is you write the above example in a shortified form"; "shortified" is not a word; perhaps "so you can write this example in a more succinct form". * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i... - The note "odeint already includes all the code presented here, see gsl_wrapper.hpp, so gsl_vectors can be used straight out-of-box. The following description is just for educational purpose." is duplicated in the subsequent paragraph. - Using x/y for element-wise division in vector_space_algebra seems potentially contentious. Similarly for abs. Perhaps it would be better to provide a customization point in boost::numeric::odeint as you have done with vector_space_reduce, whose default implementations use / and abs as in the current scheme. That way people will not be required to implement potentially misleading operators for their types. - "vector_space_reduce_impl< state_type >::reduce( state , operator , init )"; you probably shouldn't use the keyword "operator" as an identifier here. * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i... - "the state_type of the stepper must not necessarily be used to call the do_step method"; I'm not sure what this means; should it be "the state_type of the stepper need not necessarily be used to call the do_step method" * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/concepts... - Concepts are normally written without underscores, so SymplecticSystem rather than Symplectic_System. * http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/concepts... - In various places you have ScaleSum/N/ with literal '/' characters, where I presume they were intended to be formatting to make 'N' italics. - Rather than having a dedicated array_algebra, could you not have range_algebra automatically switch to the more efficient implementation when it detects a suitable State type? I hope to get a chance to play with the library later and submit a proper review. For now, congratulations on an impressive library. John Bytheway
Hi John, thank you for reading through the docs. Here are some comments. Of course most of your comments on spelling, grammar and formatting are right and we will change the docs.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_...
- "They also occur if partial differential equations (PDEs) are discretized in one coordinate."; wouldn't you have to discretize in all but one coordinate?
You are right, we will change this.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_...
- I see no mention of any headers except the global one boost/numeric/odeint.hpp. Is it useful or possible to include only parts of the library?
It is possible to use only parts of the library. We will put a paragraph explaining how to use only parts into this section.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/getting_...
- "a simple function calculating f(x)'"; I think you mean either "f(x)" or "x'", not "f(x)'".
Right, we will change it.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial...
- There's one case here where you use "--" as an em dash. "on the example of explicit_error_rk54_ck -- a 5th order Runge-Kutta method". There are various other places throughout the docs which use just "-" for em dash. It would be best if you could use a proper em dash character, but in any case you should be consistent.
ok, we wil change this. It comes from LaTeX notation which we usually use for writting documents.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/tutorial...
- "the first numerical experiment which has been implemented on a computer in 1953. It was studied at Los Alamos on one of the first computer (a MANIAC I) and it triggered a whole new tree of mathematical and physical science."; This is ambiguous (it could mean the first experiment amongst those in 1953). If you really mean it was the first computational numerics experiment then you should probably say something like "the first numerical experiment to be implemented on a computer. It was studied at Los Alamos in 1953 on one of the first computers (a MANIAC I) and triggered a whole new tree of mathematical and physical science."
Yu are right, we wil change it.
- "For example the unit test for Boost.Units take up to 4 GB of memory at compilation."; are you referring to the unit tests of Boost.Units, or a unit test of odeint that uses Boost.Units? Please clarify.
The test with odeint and Boost.Units are meant. We will clarify.
- The matrix-as-state example has a bug:
for( size_t j=0 ; j<x.size2() ; ++j ) dxdt( 0 , j ) = dxdt( 0 , x.size1() -1 ) = 0.0;
should be
for( size_t j=0 ; j<x.size2() ; ++j ) dxdt( 0 , j ) = dxdt( x.size1() -1, j ) = 0.0;
I think you are right. I will check this.
This bug makes me think that you ought to provide a debug mode where every entry in dxdt is set to NaN before calling the system operator(), and then they are checked to be non-NaN afterwards. Of course, this would only work for types with a NaN value, but I think all your examples do have that (except maybe the multiprecision types).
Yes, that might be possible. It sounds like a good idea. But it might not work if someone uses a multiprecision type, or if your state type is vor example a vector of complex numbers.
- "for full support the min( x , y ), max( x , y ), pow( y , y ) must be callable"; what are x and y? Is it significant that pow has both arguments equal but min and max don't? I hope the requirements on the numeric type are laid out more formally elsewhere; please link to that.
ok, we will clarify this. And it should be pow( x , y ).
- In the section on controlled steppers, the formulae are getting sufficiently complex that it might be worth trying to use TeX-generated pngs rather than straight HTML (as is done in Boost.Math, e.g. <http://www.boost.org/doc/libs/1_51_0/libs/math/doc/sf_and_dist/html/math_toolkit/dist/stat_tut/weg/st_eg/tut_mean_intervals.html>)
ok, we will check this.
- I can't figure out what the three lines after table 1.6 are about.
- The long line "boost::numeric::odeint::result_of::make_dense_output< r..." in Dense output steppers is too long for my screen; can you break it?
- The resizing policy names are a bit clumsy; I think always_resize, initially_resize, and never_resize might be better.
you are right. I am also not happy with their names. But renaming them might break some code. Of course, one can also introduce some typedef for backward compatibility.
- "Typical use cases for this kind of resizer are self expanding lattices like shown in the tutorial"; can you link to the relevant example in the tutorial?
Sure, we will do.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i...
- Personally I think the ranges are more useful than the iterators, so it would be nice if they were mentioned in the section title (e.g. "Iterators and Ranges")
You are right. Iterators and ranges sound also good for me.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i...
- The note "odeint already includes all the code presented here, see gsl_wrapper.hpp, so gsl_vectors can be used straight out-of-box. The following description is just for educational purpose." is duplicated in the subsequent paragraph.
- Using x/y for element-wise division in vector_space_algebra seems potentially contentious. Similarly for abs. Perhaps it would be better to provide a customization point in boost::numeric::odeint as you have done with vector_space_reduce, whose default implementations use / and abs as in the current scheme. That way people will not be required to implement potentially misleading operators for their types.
Ok, I have to check this. I will come back to you if this is possible or not.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/odeint_i...
- "the state_type of the stepper must not necessarily be used to call the do_step method"; I'm not sure what this means; should it be "the state_type of the stepper need not necessarily be used to call the do_step method"
Yes, this meant. We will try to clarify this.
* http://headmyshoulder.github.com/odeint-v2/doc/boost_numeric_odeint/concepts...
- In various places you have ScaleSum/N/ with literal '/' characters, where I presume they were intended to be formatting to make 'N' italics.
- Rather than having a dedicated array_algebra, could you not have range_algebra automatically switch to the more efficient implementation when it detects a suitable State type?
Yes, in principle this is possible. You can extend the idea to introduce a algebra and operations dispatcher which finwd the appropirate algebra for every state type not only for ranges or arrays, but also for Thrust's vector types or Boost.Fusion sequences for example. If people really like to have such a feature we can implement this.
On 22/09/12 13:41, Karsten Ahnert wrote:
- The resizing policy names are a bit clumsy; I think always_resize, initially_resize, and never_resize might be better.
you are right. I am also not happy with their names. But renaming them might break some code. Of course, one can also introduce some typedef for backward compatibility.
Yes, I was thinking of something like that.
- Rather than having a dedicated array_algebra, could you not have range_algebra automatically switch to the more efficient implementation when it detects a suitable State type?
Yes, in principle this is possible. You can extend the idea to introduce a algebra and operations dispatcher which finwd the appropirate algebra for every state type not only for ranges or arrays, but also for Thrust's vector types or Boost.Fusion sequences for example. If people really like to have such a feature we can implement this.
These aren't quite the same thing. I'm suggesting an optimization, rather than a notational convenience. It shouldn't change observable behaviour. The more general idea of automatically dispatching to the right algebra is more dangerous, because for example it is entirely possible to have a type which is both a Fusion sequence and a Range (but with different semantics in each interpretation). So, I am not suggesting that (though I wouldn't argue against it either). John
John Bytheway <jbytheway+boost@gmail.com> wrote:
- The resizing policy names are a bit clumsy; I think always_resize, initially_resize, and never_resize might be better.
you are right. I am also not happy with their names. But renaming
On 22/09/12 13:41, Karsten Ahnert wrote: them
might break some code. Of course, one can also introduce some typedef for backward compatibility.
Yes, I was thinking of something like that.
- Rather than having a dedicated array_algebra, could you not have range_algebra automatically switch to the more efficient implementation when it detects a suitable State type?
Yes, in principle this is possible. You can extend the idea to introduce a algebra and operations dispatcher which finwd the appropirate algebra for every state type not only for ranges or arrays, but also for Thrust's vector types or Boost.Fusion sequences for example. If people really like to have such a feature we can implement this.
These aren't quite the same thing. I'm suggesting an optimization, rather than a notational convenience. It shouldn't change observable behaviour.
So you mean essentially the range algebra should forward to the array algebra when it encounters an array? At some point there has to be an automatic deduction. At the moment the range algebra is the standard and whenever something else is wanted it has to be user-specified. I think it might be reasonable to give an automatic mechanism, but allow for overriding by explicit definition. The more general idea of automatically dispatching to the
right algebra is more dangerous, because for example it is entirely possible to have a type which is both a Fusion sequence and a Range (but with different semantics in each interpretation). So, I am not suggesting that (though I wouldn't argue against it either).
This would be an edge case where explicit user definition is required, but i think in 99% an algebra dispatcher would be sufficient. Mario
John
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 25/09/12 22:38, Mario Mulansky wrote:
John Bytheway <jbytheway+boost@gmail.com> wrote:
The more general idea of automatically dispatching to the right algebra is more dangerous, because for example it is entirely possible to have a type which is both a Fusion sequence and a Range (but with different semantics in each interpretation). So, I am not suggesting that (though I wouldn't argue against it either).
This would be an edge case where explicit user definition is required, but i think in 99% an algebra dispatcher would be sufficient.
On further reflection I thought of a particular case where this happens. Any std::pair<Iterator, Iterator> is both a Fusion Sequence and a Range, with very different semantics. Just something to consider. John
On Wednesday, September 26, 2012 06:42:09 PM John Bytheway wrote:
On 25/09/12 22:38, Mario Mulansky wrote:
John Bytheway <jbytheway+boost@gmail.com> wrote:
The more general idea of automatically dispatching to the right algebra is more dangerous, because for example it is entirely possible to have a type which is both a Fusion sequence and a Range (but with different semantics in each interpretation). So, I am not suggesting that (though I wouldn't argue against it either).
This would be an edge case where explicit user definition is required, but i think in 99% an algebra dispatcher would be sufficient.
On further reflection I thought of a particular case where this happens. Any std::pair<Iterator, Iterator> is both a Fusion Sequence and a Range, with very different semantics. Just something to consider.
True, one should be very defensive with automatically chosing the fusion algebra. However, I like the idea of automatically choosing the array algebra when possible, and I also still think the appropriate point to make this decision would be a general algebra dispatcher. Mario
On 09/27/2012 01:42 AM, John Bytheway wrote:
On 25/09/12 22:38, Mario Mulansky wrote:
John Bytheway <jbytheway+boost@gmail.com> wrote:
The more general idea of automatically dispatching to the right algebra is more dangerous, because for example it is entirely possible to have a type which is both a Fusion sequence and a Range (but with different semantics in each interpretation). So, I am not suggesting that (though I wouldn't argue against it either).
This would be an edge case where explicit user definition is required, but i think in 99% an algebra dispatcher would be sufficient.
On further reflection I thought of a particular case where this happens. Any std::pair<Iterator, Iterator> is both a Fusion Sequence and a Range, with very different semantics. Just something to consider.
Having a range as state_type is difficult maybe even impossible, since ranges can not be resized. A range models something that already exists and inside the stepper a temporary must be created. (But a range can be passed to the do_step method.) Nevertheless, it is possible the create a dispatcher like template< class State , class Enabler = void > struct algebra_dispatcher; template< class T , class A > struct algebra_dispatcher< std::vector< T , A > , void > { typedef range_algebra type; }; // this will create errors for ranges of pairs of iterators template< class R > struct algebra_dispatcher< R , typename boost::fusion::is_sequence< R >::type > { typedef fusion_algebra type; }; The steppers simply get the value of the algebra dispatcher as a default parameter: template< class State , ... , class Algebra = typename algebra_dispatcher< State >::type , ... > struct stepper { ... }; Then, the user will mostly get the right algebra, and in case this is not possible he can easily specify the desired one. (The same should be done for the operations.)
participants (3)
-
John Bytheway -
Karsten Ahnert -
Mario Mulansky