eddelbuettel / bh

R package providing Boost Header files
85 stars 33 forks source link

Path length adjustments break certain includes #67

Closed ashiklom closed 4 years ago

ashiklom commented 4 years ago

For example, in numeric/odeint.hpp:

https://github.com/eddelbuettel/bh/blob/f84296a8693e8b533db09928a003d053c7722a5d/inst/include/boost/numeric/odeint.hpp#L31

...but after path length adjustment, the actual file is numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp: https://github.com/eddelbuettel/bh/blob/master/inst/include/boost/numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp

There may be other examples, but this is the first one that I ran into when compiling.

ashiklom commented 4 years ago

Another place this breaks is here:

https://github.com/eddelbuettel/bh/blob/f84296a8693e8b533db09928a003d053c7722a5d/inst/include/boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp#L23

eddelbuettel commented 4 years ago

Yes.

Please see the file ChangeLog. I have to adjust those by hand because R insists on using a tar limit of 102 char which was last relevant a decade ago ....

And I checked and think I did but may have missed one or two. We can (and will) clean this up. Thanks for the help.

eddelbuettel commented 4 years ago

I think I was trying to only renamed in generation/ but my mistake also renamed one above:

edd@rob:~/git/bh/inst/include/boost/numeric/odeint/stepper(master)$ git diff --cached
diff --git a/inst/include/boost/numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp b/inst/include/boost/numeric/odeint/stepper/runge_kutta_cash_karp54_classic.hpp
similarity index 100%
rename from inst/include/boost/numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp
rename to inst/include/boost/numeric/odeint/stepper/runge_kutta_cash_karp54_classic.hpp
edd@rob:~/git/bh/inst/include/boost/numeric/odeint/stepper(master)$ 

This should fix it.

Coincidentally, how did you find it? Because the reverse depends checks I ran (against 160+) package and which CRAN ran did not.

eddelbuettel commented 4 years ago

With that correction / reversal to what was there and needs to be there we have

edd@rob:~/git/bh/inst/include/boost/numeric/odeint/stepper(master)$ ag generation_runge_kutta_cash_karp54_cl.hpp 
generation.hpp
27:#include <boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp>
edd@rob:~/git/bh/inst/include/boost/numeric/odeint/stepper(master)$ ls -1 generation/
generation_controlled_adams_b_m.hpp
generation_controlled_runge_kutta.hpp
generation_dense_output_runge_kutta.hpp
generation_rosenbrock4.hpp
generation_runge_kutta_cash_karp54_cl.hpp
generation_runge_kutta_cash_karp54.hpp
generation_runge_kutta_dopri5.hpp
generation_runge_kutta_fehlberg78.hpp
make_controlled.hpp
make_dense_output.hpp
edd@rob:~/git/bh/inst/include/boost/numeric/odeint/stepper(master)$ 

Correct?

ashiklom commented 4 years ago

Thanks for the quick reply! Yes, I think renaming back to the full classic file should fix things, but FWIW, I applied the following patch to get this working for our specific use case:

diff --git a/inst/include/boost/numeric/odeint.hpp b/inst/include/boost/numeric/odeint.hpp
index 642e46fe..ffbbd61f 100644
--- a/inst/include/boost/numeric/odeint.hpp
+++ b/inst/include/boost/numeric/odeint.hpp
@@ -28,7 +28,7 @@
 #include <boost/numeric/odeint/stepper/runge_kutta4_classic.hpp>
 #include <boost/numeric/odeint/stepper/runge_kutta4.hpp>
 #include <boost/numeric/odeint/stepper/runge_kutta_cash_karp54.hpp>
-#include <boost/numeric/odeint/stepper/runge_kutta_cash_karp54_classic.hpp>
+#include <boost/numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp>
 #include <boost/numeric/odeint/stepper/runge_kutta_dopri5.hpp>
 #include <boost/numeric/odeint/stepper/runge_kutta_fehlberg78.hpp>

diff --git a/inst/include/boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp b/inst/include/boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp
index 1cc0f91d..3e4a9e33 100644
--- a/inst/include/boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp
+++ b/inst/include/boost/numeric/odeint/stepper/generation/generation_runge_kutta_cash_karp54_cl.hpp
@@ -20,7 +20,7 @@
 #define BOOST_NUMERIC_ODEINT_STEPPER_GENERATION_GENERATION_RUNGE_KUTTA_CASH_KARP54_CLASSIC_HPP_INCLUDED

 #include <boost/numeric/odeint/stepper/controlled_runge_kutta.hpp>
-#include <boost/numeric/odeint/stepper/runge_kutta_cash_karp54_classic.hpp>
+#include <boost/numeric/odeint/stepper/runge_kutta_cash_karp54_cl.hpp>
 #include <boost/numeric/odeint/stepper/generation/make_controlled.hpp>

There may be other places that this needs to be fixed as well that our code doesn't rely on.

how did you find it?

We have a package that isn't on CRAN (https://github.com/jgcri/hector) that uses some Boost ODE functionality. I discovered this after updating BH today and trying to rebuild it.

eddelbuettel commented 4 years ago

Working on it now on my side. I really appreciate you found that. I will have a candidate package for you in a few, and the I have to ask you to get your package onto CRAN as a test :)

FWIW I think you diff above is wrong. Reason? stepper/runge_kutta_cash_karp54_cl.hpp should not have been renamed, so the fix is NOT to make further changes but to undo the renaming. Which I have done (but not yet pushed). In that case we need no further renames. Do you agree?

Could you test? Candidate package here: http://dirk.eddelbuettel.com/tmp/BH_1.72.0-2.tar.gz

ashiklom commented 4 years ago

FWIW I think you diff above is wrong. Reason? stepper/runge_kutta_cash_karp54_cl.hpp should not have been renamed, so the fix is NOT to make further changes but to undo the renaming. Which I have done (but not yet pushed). In that case we need no further renames. Do you agree?

Whatever you think is best! Your solution does seem more straightforward.

Could you test?

Just tested with the patched version -- seems to work great! Thanks for the quick fix!

eddelbuettel commented 4 years ago

Awesomeness! Thanks again, this will go to CRAN and should hopefully hit the mirrors "soon".