Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #156 for gsl_odeiv_system #172

Merged
merged 7 commits into from
May 15, 2020
Merged

Conversation

hakonhagland
Copy link
Contributor

Hello. After a long struggle, I finally managed to implement some typemaps for Math::GSL::ODEIV to try to fix issue #156 for gsl_odeiv_system and gsl_odeiv_evolve_apply(). I am still learning XS and swig so please let me know how to improve this :)

@leto
Copy link
Owner

leto commented Oct 13, 2019

@hakonhagland wow, this is impressive if it's your first foray into XS/SWIG land! I will see what Travis says about the tests, but I am very interested to merge this

@leto
Copy link
Owner

leto commented Oct 13, 2019

@hakonhagland thanks for fixing 5.8 support!

It seems that Travis has lots of PASS except for GSL master branch, which I think could be our test suite potentially being broken on GSL master branch. Not sure if that is your fault, but once we can clear that up, I am happy to merge this awesome PR 👍

@hakonhagland
Copy link
Contributor Author

@leto Thanks positive feedback! I am looking at the build failure for the master branch (line #5079) https://travis-ci.org/leto/math--gsl/jobs/597321360#L5079 : it says

Unsupported GSL version!!! : 2.6 at Build.PL line 77.

it seems to me this comes from line #323 in Ver2Func.pm : https://github.com/leto/math--gsl/blob/master/inc/Ver2Func.pm#L323 and this is because there is no 2.6 key in the array @ver2func at line #10 : https://github.com/leto/math--gsl/blob/master/inc/Ver2Func.pm#L10

@leto
Copy link
Owner

leto commented Oct 14, 2019

@hakonhagland you are right! Our code needs to learn about version 2.6, and the associated new functions, so it can do Magic

@leto
Copy link
Owner

leto commented Oct 14, 2019

@hakonhagland would you mind adding a version 2.6 key to see if we can get Travis happy for your PR?

@hakonhagland
Copy link
Contributor Author

@leto Out of curiosity: why do we test against gsl-master in the travis script anyway? If we know we don't support the latest version, why test against it?

@leto
Copy link
Owner

leto commented Oct 14, 2019

@leto we test against it so we can detect breakages before a release of GSL comes out. Instead our test suite breaks sooner, giving us time to become compatible for a GSL release. This let's us be prepared, instead of users coming to us and saying "The new GSL release doesn't work with Math::GSL". In theory,
our test suite should pass on GSL master, except for when we are learning of brand new GSL code and reacting to that failure.

So it's not that we "know we don't support it", our goal is to support it and that means test failures exactly when GSL adds/changes stuff, which is more appropriate than much later, from Math::GSL users.

We should probably improve our test code to give a more useful error? Any suggestions are welcome.

@leto
Copy link
Owner

leto commented Oct 14, 2019

@hakonhagland the reason this is happening to you is because there has not been a lot of Math::GSL development, so our test suite became a bit old and the new 2.6 version of GSL came out without anybody updating Math::GSL codebase. If you can think of ways to improve things, so this manually update stuff isn't needed, please do tell 😸

@leto
Copy link
Owner

leto commented May 9, 2020

@hakonhagland I think now maybe this is merge-able or able for you to update for 2.6 and send a new PR

Implements some typemaps for Math::GSL::ODEIV to fix issue leto#156 for gsl_odeiv_system and
gsl_odeiv_evolve_apply().
To compile on perls earlier than 5.18, change the call to av_top_index() with a call to av_len()
mPUSHs was introduced in 5.8.9. Replace with the PUSHs macro to provide
backward compatibility.
@hakonhagland
Copy link
Contributor Author

rebased onto current master

@hakonhagland
Copy link
Contributor Author

Forgot to add ODEIV_evolve.t to MANIFEST so the test did not run for the travis build.

Copy link
Owner

@leto leto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@leto leto merged commit ec1afb4 into leto:master May 15, 2020
@leto
Copy link
Owner

leto commented May 15, 2020

@hakonhagland thank you for going on an Odyssey to fix this!!! It's really exciting stuff. Also, this code really complements my Math::ODE module, a pure-Perl module that implements RK4. I think it would be super interesting to compare what the code looks like to solve the same equations with both, and also have CPU timings and even memory use if that is easy.

Many numerics people will likely be interested to use your new code and showing the actual performance numbers and providing a "Rosetta stone" to convert Math::GSL programs to Math::GSL::ODEIV programs would be cool. We may be able to do it automatically!

The Math::ODE tests are here, they are quite simple: https://github.com/leto/math--ode/tree/master/t

Another thing we can do, is compare error estimates across both modules for the same exact equations. You can even vary step size if you want to study that too. I haven't checked but it's likely Math::GSL internally uses RK8 instead of RK4 and so it's should be crazy more accurate and fast.

@morgantomj
Copy link

morgantomj commented May 15, 2020 via email

@leto
Copy link
Owner

leto commented May 15, 2020

@morgantomj thanks!! I hope Math::GSL is useful to you and everybody else that uses it. It's definitely the most complex and oldest codebase that I actively maintain! I never knew how long this ride was going to be when I started 😅

@hakonhagland
Copy link
Contributor Author

thank you for going on an Odyssey to fix this

@leto You are very welcome! I have learnt a lot and it was fun. I think it would be useful to add a wiki page (for developers) explaining the steps needed to upgrade to a new GSL version. It was a trial and error approach for me, and I think the task would have been much easier if there was some kind of a recipe.

@hakonhagland
Copy link
Contributor Author

I'd like to send my Kudos to both of you

@morgantomj Thank you.

@leto
Copy link
Owner

leto commented May 15, 2020

@hakonhagland @morgantomj you both may enjoy this relic of the past I just found: http://leto.net/code/Math-GSL/

And yes, developer docs are absolutely crucial. Math::GSL has one of the most complex build systems of anything that I know of, and definitely one of the most complex build systems in all of CPAN.

Fun fact: I started the original Build.PL from https://github.com/rocky/Perl-Device-Cdio but we evolved to be way more complex, because GSL is so complex and ever-changing!

@hakonhagland
Copy link
Contributor Author

I think it would be super interesting to compare what the code looks like to solve the same equations with both, and also have CPU timings and even memory use if that is easy.

@leto I sent you an email but seems like I got the wrong email address. The response was "The recipient server did not accept our requests to connect"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants