search

eddelbuettel / rquantlib

R interface to the QuantLib library
117 stars 50 forks source link

Change rgl.* calls to *3d calls. #174

Closed dmurdoch closed 1 year ago

dmurdoch commented 1 year ago

The surface3d() function needs an rgl fix that's in the about-to-be-submitted version 0.111.5, so I've added that version requirement. Let me know if you'd prefer a run-time test and workaround for the old code.

eddelbuettel commented 1 year ago

Thanks for the changes. I am always very hesitant to add Suggests: rgl (>= 0.111.5) when such a version is not yet on CRAN and personally prefer to test in code if the required version is enough, and politely exit with a user-facing note when not.

Here it may not matter as the CI default is to ignore Suggests: (as I commonly do, that is a long story in and by itself you also correctly harped upon on one of the lists (== we collectively agree to minimize test surface here to keep tests in time limits, which is somewhat contrary to having tests in the first place yada yada ... but CRAN time is finite yada yada).

eddelbuettel commented 1 year ago

The one thing I always ask about is to add an extry to ChangeLog. Can you do that? Standard old school GNU format with date, name, email separated by two spaces, bullets indented by eight or a tab.

dmurdoch commented 1 year ago

This commit should work with current CRAN rgl as well as the upcoming one. After 0.111.5 is released, the version check code could be removed.

eddelbuettel commented 1 year ago

~Sorry for the hoops but I cannot undo 'red' if you mark the issue as resolved, methinks. If I raised maybe they want me to clear it:~

Never mind, my bad.

Nice PR otherwise, thanks for all the work.

dmurdoch commented 1 year ago

Thanks!

eddelbuettel commented 1 year ago

I got distracted waiting for the CI and then forgot about the pending PR -- thanks again. The versioned check is perfect.

I am about to commit a simple fix (to create a default options array object) so that calling the viewer will work.

dmurdoch commented 1 year ago

The newer version of rgl is now on CRAN, so if you prefer a version requirement in DESCRIPTION that would work now, or you could leave in the run-time check and let people keep working with older versions.

eddelbuettel commented 1 year ago

Thanks, I saw via CRANberries so congrats on a smooth transition. I think the run-time check should do.

eddelbuettel commented 1 year ago

I got a little tied up with various other package updates (and still have some to do...) but I reckoned rgl, long updated at CRAN, is fine without a RQuantLib update as you rejigged the example to work with old and new rgl, correct?

dmurdoch commented 1 year ago

I think the RQuantlib changes should work fine with what's on CRAN now, and should be fine after the next update too. The most recent update on CRAN doesn't do the deprecation, the next one will. I've just finished going through the revdeps on CRAN to let authors know they'll get deprecation warnings as of the next release. I'm not sure when that will happen, but it'll likely be at least a week, more likely two.

eddelbuettel commented 1 year ago

Ah, that explains it -- I wasn't looking closely enough.

Especially once I have my "big fish" out of the way (Rcpp has been uploaded and I am waiting for 1 1/2 days now, RcppArmadillo and then BH are next) it would be a quick turnaround for this package. Just let me if/when you need an update.