headmyshoulder / odeint-v2

odeint - solving ordinary differential equations in c++ v2
http://headmyshoulder.github.com/odeint-v2/
Other
340 stars 101 forks source link

SFINAE specializations for Boost.Fusion sequences lead to ambiguities #128

Closed mgaunard closed 10 years ago

mgaunard commented 10 years ago

I tried to adapt odeint for my own containers (similar to MTL or Eigen), which also happen to be boost.fusion sequences, and there are ambiguities because of the specializations of is_resizeable etc.

headmyshoulder commented 10 years ago

Hi Matthias, can you be a bit more specific? Mabye you can show us some compiler errors or a test program?

headmyshoulder commented 10 years ago

odeint already provides specializations for fusion sequences.

mgaunard commented 10 years ago

That's precisely the problem.

It doesn't work when I want to specialize for my own matrix type which is also a fusion sequence (because it uses Boost.Proto)

mariomulansky commented 10 years ago

Can you give some example for your state type to give us an impression on what you are dealing with? Looking into the odeint's is_resizeable implementation for fusion sequences it is already quite clever and checks if any of the elements is resizeable and if so defines the sequence itself as resizeable. Is this not the behavior you could work with?

The current fusion backend (fusion_algebra) is only working for fusion vectors of integral types, so you might have to provide your own algebra to make odeint run.

mgaunard commented 10 years ago

The problem is that I cannot provide my own specializations. They conflict with the ones for Fusion sequences. The right way to fix this IMHO is to make all those traits be two-level so that the Fusion specializations are only enabled if nothing else matches.

headmyshoulder commented 10 years ago

Hi,

this code below works for me. What do you have in mind?

include <boost/numeric/odeint.hpp>

include

using namespace std;

typedef boost::fusion::vector< double , double > state_type;

namespace boost { namespace numeric { namespace odeint {

template<> struct same_size_impl< state_type , state_type > { static bool same_size( const state_type &x1 , const state_type &x2 ) { cout << "bla" << endl; return true; } };

} // namespace odeint } // namespace numeric } // namespace boost

int main( int argc , char *argv[] ) { state_type x1 , x2; cout << boost::numeric::odeint::same_size( x1 , x2 ) << endl; return 0; }

On 05/07/2014 07:11 PM, Mathias Gaunard wrote:

The problem is that I cannot provide my own specializations. They conflict with the ones for Fusion sequences. The right way to fix this IMHO is to make all those treats by two-level so that the Fusion specializations are only enabled if nothing else matches.

— Reply to this email directly or view it on GitHub https://github.com/headmyshoulder/odeint-v2/issues/128#issuecomment-42455009.

mariomulansky commented 10 years ago

I understand your problem, what I'm wondering is if you really need your own specialization. The current implementation tags a fusion sequence as resizeable if any of its elements is resizeable. Right now I can't think of a situation where this shouldnt work. This is most likely due to my lack of imagination, therefore it would be very helpful if I knew what your fusion sequences are made of?

headmyshoulder commented 10 years ago

Ok, my solution only works for concrete types, not template.

Yeah, maybe a two level approach would be a good solution.

headmyshoulder commented 10 years ago

Hi, I have added a sfinae branch and changed is_resizeable to a two level specialization scheme. Can you check that is fulfils your requirements? Then I will change the other classes. Is the naming of is_resizable_sfinae ok for you?

mgaunard commented 10 years ago

My understanding is that to make odeint efficient, I need to at least specialize all of the following:

vector_space_norm_inf
algebra_dispatcher
is_resizeable
same_size_impl
resize_impl
copy_impl

at least the following have SFINAE fusion specializations that lead to ambiguities in my case:

is_resizeable
resize_impl
same_size_impl
algebra_dispatcher

While it could be argued that we could design our code so that the default resize/same_size implemntations do the right thing, that seems difficult for algebra_dispatcher.

mgaunard commented 10 years ago

The approach on the branch looks good (though you have left some of the original code as commented which might not be desirable).

headmyshoulder commented 10 years ago

On 05/13/2014 07:14 PM, Mathias Gaunard wrote:

The approach on the branch looks good (though you have left some of the original code as commented which might not be desirable).

Ok, I will implement the other classes with this two stage approach. Of course, the commented code will dispear. It was a quick trial on my way home from the office :)

— Reply to this email directly or view it on GitHub https://github.com/headmyshoulder/odeint-v2/issues/128#issuecomment-42984183.

headmyshoulder commented 10 years ago

On 05/13/2014 07:13 PM, Mathias Gaunard wrote:

My understanding is that to make odeint efficient, I need to at least specialize all of the following:

vector_space_norm_inf algebra_dispatcher is_resizeable same_size_impl resize_impl copy_impl

at least the following have SFINAE fusion specializations that lead to ambiguities in my case:

is_resizeable resize_impl same_size_impl algebra_dispatcher

While it could be argued that we could design our code so that the default resize/same_size implemntations do the right thing, that seems difficult for algebra_dispatcher.

I think algebra_dispatcher is uncritical. You can always instantiate any stepper with the desired algebra

runge_kutta4< state_type , double , state_type , double , your_algebra , your_operations > stepper;

algebra_dispatcher is only for convenience, such that the user does not need to specify all types for the stepper.

I also think that it is not necessary to specialize copy_impl if your type has a copy contructor, or a copy assignment operator.

— Reply to this email directly or view it on GitHub https://github.com/headmyshoulder/odeint-v2/issues/128#issuecomment-42983987.

headmyshoulder commented 10 years ago

I changes the remaining class for a two level specialization. Can you check if it works for you? If everthing is fine I would merge the changes into the trunk.

mgaunard commented 10 years ago

It works fine.

alankelly commented 10 years ago

HI,

Any ETA on merging this? I'm about to make another pull request which depends on this.

Thanks

headmyshoulder commented 10 years ago

Uups, I thought I have already merged this. I will do it on Sunday evening.

On 27. Juni 2014 16:08:12 MESZ, alankelly notifications@github.com wrote:

HI,

Any ETA on merging this? I'm about to make another pull request which depends on this.

Thanks


Reply to this email directly or view it on GitHub: https://github.com/headmyshoulder/odeint-v2/issues/128#issuecomment-47348148

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

headmyshoulder commented 10 years ago

Ok, I have merge this branch into the master branch.

On 06/27/2014 04:08 PM, alankelly wrote:

HI,

Any ETA on merging this? I'm about to make another pull request which depends on this.

Thanks

— Reply to this email directly or view it on GitHub https://github.com/headmyshoulder/odeint-v2/issues/128#issuecomment-47348148.

mariomulansky commented 10 years ago

can this be closed then?

alankelly commented 10 years ago

Yep - but I didn't open it so I can't.