Possible change / correction of Field export affecting four packages

@coatless

Issue #263, opened quite some time ago by @coatless, demonstrated that we had a bug stacked away in dealing with a field exporter which dropped a dimension. I worked a bit on that in November and committed a fix albeit behind a #define. I had meant to come back to this and had time last week to run two sets of 'before' and 'after' reverse-depends checks to assess how costly it would be to enable this unconditionally.

It turns out that four (out of 950 total (!!)) packages then fail tests:

AlphaSimR by @gaynorr et al -- already addressed in version 1.1.2 on CRAN
carat by @yexiaoqingruc et al
odpc by @esmucler -- will revert to old behavior, see #3, #4, addressed in version 2.0.5 on CRAN
perccal by Dan McCarthy (not on GH, emailed 2022-02-13)

I have not yet had time to look into patches or changes. We probably want to fix this, and worst case can offer a #define to allow these four packages (and other uses like them) to get the old behavior back (in the sense of 'we published an API so we need to honor that contract') but otherwise offer 'better' code.

So "just" opening this issue to see if we can start a dialogue about what would be a suitable timeline to make the change. Maybe in three months? Too soon?

It's a quick fix for me, so I'll plan on submitting a new version of AlphaSimR to CRAN this week that works with both versions of RcppArmadillo.

Thanks so much for the prompt reply -- I haven't even put the 'final' change in, and I guess I would need to give you way to determine 'which version you are encountering'. So no rush, it would be best to hear from everybody. I'd like to exclude first that we're not creating an issue (though we should be able to set it to always be able to opt into the current behavior).

Ok, I at least update the master branch now to what I had been testing.

What would help you in terms of determining whether you are building under 'old' or 'new' behavior? Is just the version number for RcppArmadillo good enough? Or do we need more?

@gaynorr

Follow-up: I ran another full reverse-depends check over night.

The very good news is that our count is down from four to three. I am very grateful to @gaynorr and the AlphaSimR team for already taking care of this. The three packages identified in the first run are still affected. As we have not heard from their authors / maintainers I will switch to email and CC the CRAN team.

So here is what I think makes sense and what I am going to propose in email as well:

we keep the status quo at RcppArmadillo for a few more weeks to give users time for making changes
the corrected behavior can already be opted into by defining RCPP_ARMADILLO_FIX_Field
a suggested time frame for a change is two+ months: this will not be change until after May 1
at the first RcppArmadillo release CRAN after May 1, I will flip the switch to the corrected Fields export
packages wishing to maintain the current status quo (of slightly buggy but 'known' behavior) will then get it back by defining RCPP_ARMADILLO_OLD_Field_BEHAVIOR

I hope this works for everybody. Please comment here (or reply to the to-be-sent email) with comments or questions.

Hi Dirk, thanks for the heads up. I'll work on the fix for odpc in the next few weeks. Best Ezequiel

…

On Wed, Feb 23, 2022, 19:16 Dirk Eddelbuettel ***@***.***> wrote: Follow-up: I ran another full reverse-depends check over night. The very good news is that our count is down from four to three. I am very grateful to @gaynorr <https://github.com/gaynorr> and the AlphaSimR team for already taking care of this. The three packages identified in the first run are still affected. As we have not heard from their authors / maintainers I will switch to email and CC the CRAN team. So here is what I think makes sense and what I am going to propose in email as well: - we keep the status quo at RcppArmadillo for a few more weeks to give users time for making changes - the corrected behavior can already be opted into by defining RCPP_ARMADILLO_FIX_Field - a suggested time frame for a change is two+ months: this will not be change until after May 1 - at the first RcppArmadillo release CRAN after May 1, I will flip the switch to the corrected Fields export - packages wishing to maintain the current status quo (of slightly buggy but 'known' behavior) will then get it back by defining RCPP_ARMADILLO_OLD_Field_BEHAVIOR I hope this works for everybody. Please comment here (or reply to the to-be-sent email) with comments or questions. — Reply to this email directly, view it on GitHub <#364 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADLNQX4MC3JQHOHPGWPY65TU4UQABANCNFSM5OJXLO7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *This information may be confidential or privileged. Any unauthorized disclosure is strictly prohibited. Your data has been incorporated into our privacy system to manage your request. You can exercise your personal data rights and check our Privacy Policy here <https://glovoapp.com/es/legal/privacy>. *

Super -- let me know if you need help with a particular RcppArmadillo interim release to check the value on way or another. It really only affects on short statemement in the header defining wrap().

That would be great, could you tell me version I should test this against? I added the #define you provided in the main headers file for the project here and I think that should do it.

Sure -- the change to src/config.h looks good (if and and when that file is always sources before RcppArmadillo.h which I cannot see from the commit...)

Actually spot-checking getMatrices.h it is NOT good enough as

https://github.com/esmucler/odpc/blob/b8dabea1625027a5a3b334a938f94174c0e92a32/src/getMatrices.h#L4-L8

include config.h after it includes RcppArmadillo.h -- that is too late. You need to set the define before you include the (catch-all) header for RcppArmadillo and you have to do that for every file. You should see a change in behaviour as I saw your (unmodified) package fail its test when I forced this change, hence my reaching out to you.

I have not checked your other files. If you feel unsure all this I could prepare a PR with just this change. Let me know if it would help.

If you could prepare a PR with this I would be very grateful, yes!

Coming right up this weekend. I'll try to be minimal.

Navigation Menu

Repository navigation

Possible change / correction of Field export affecting four packages #364

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Possible change / correction of Field export affecting four packages #364

Description

Activity

gaynorr commented on Feb 14, 2022

eddelbuettel commented on Feb 14, 2022

eddelbuettel commented on Feb 15, 2022

eddelbuettel commented on Feb 23, 2022

esmucler commented on Feb 23, 2022

eddelbuettel commented on Feb 23, 2022

esmucler commented on Feb 25, 2022

eddelbuettel commented on Feb 25, 2022

esmucler commented on Feb 25, 2022

eddelbuettel commented on Feb 25, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions