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

Package back to CRAN? #41

Open
SebKrantz opened this issue Dec 4, 2020 · 34 comments
Open

Package back to CRAN? #41

SebKrantz opened this issue Dec 4, 2020 · 34 comments

Comments

@SebKrantz
Copy link

Hi, collapse currently has a soft dependency on lfe. I see this package was just removed from CRAN. Could it be added back?

Best regards,

Sebastian

@MatthieuStigler
Copy link
Contributor

MatthieuStigler commented Dec 5, 2020

I cannot see the CRAN check errors: https://cran-archive.r-project.org/web/checks/2020/2020-12-04_check_results_lfe.html for r-patched-solaris-x86, is anyone having the same issue? I also emailed cran-sysadmin@r-project.org

Running tests on my local machine, I see that these are just warning for numerical inconsistencies between output tests from the examples. This comes from the file /tests/Examples/lfe-Ex.Rout.save. That file itself is quite unusual to me, I fail to see the corresponding lfe-Ex.R. Could it be that it was simply not re-run recently to update changes?

Here is the output I get with R 4.0.3 and lfe 2.8.4:

 checking differences from ‘lfe-Ex.Rout’ to ‘lfe-Ex.Rout.save’ ...
   
   38c38
   <  0.008017737  0.934873578  1.301275975  0.008843272  0.081184357  0.058118985 
   ---
   >  0.008569979  0.933723478  1.297425684  0.009432572  0.082334457  0.061969275 
   40c40
   < -0.004131701 
   ---
   > -0.004721002 
Unfold full output

 checking differences from ‘lfe-Ex.Rout’ to ‘lfe-Ex.Rout.save’ ...

38c38
< 0.008017737 0.934873578 1.301275975 0.008843272 0.081184357 0.058118985

0.008569979 0.933723478 1.297425684 0.009432572 0.082334457 0.061969275
40c40
< -0.004131701


-0.004721002
654,656c654,656
< id 1.031372636 0.006691582 -0.006322827
< firm 0.006691582 1.049086528 0.010535564
< foo -0.006322827 0.010535564 1.057014174


id 1.031163938 0.006729247 -0.006303323
firm 0.006729247 1.046116404 0.010550689
foo -0.006303323 0.010550689 1.058185345
659,661c659,661
< id 3.054932e-02 -0.0001717782 -9.118383e-05
< firm -1.717782e-04 0.0194414181 -1.092769e-04
< foo -9.118383e-05 -0.0001092769 1.442915e-02


id 0.0307580151 -0.0002094436 -0.0001106874
firm -0.0002094436 0.0224115429 -0.0001244024
foo -0.0001106874 -0.0001244024 0.0132579840
666,668c666,668
< id 1.000000000 0.006433027 -0.006055682
< firm 0.006433027 1.000000000 0.010004876
< foo -0.006055682 0.010004876 1.000000000


id 1.000000000 0.006479069 -0.006034272
firm 0.006479069 1.000000000 0.010027899
foo -0.006034272 0.010027899 1.000000000
693,697c693,697
< f.1 0.9569311 185 1 f 1 0.07749880 0.07535548
< f.2 1.9949066 223 1 f 2 0.06038432 0.06860181
< f.3 3.0326896 185 1 f 3 0.07696251 0.07532290
< f.4 4.0294033 196 1 f 4 0.07527526 0.07317074
< f.5 4.9413841 211 1 f 5 0.07015206 0.07054858


f.1 0.9569311 185 1 f 1 0.07807322 0.07535548
f.2 1.9949066 223 1 f 2 0.06163607 0.06860181
f.3 3.0326896 185 1 f 3 0.07650146 0.07532290
f.4 4.0294033 196 1 f 4 0.07595171 0.07317074
f.5 4.9413841 211 1 f 5 0.07077708 0.07054858
1089c1089
< [1] 0.6600197


[1] 0.6590496
1091c1091
< [1] 0.03292458


[1] 0.03281585
1093c1093
< [1] 11040


[1] 11102
1099c1099
< [1] 499.8301


[1] 503.8349
1101c1101
< [1] 3.43084


[1] 4.485407
1103c1103
< [1] 56


[1] 28
1106c1106
< [1] 495


[1] 503
1157a1158,1164

No test:

the covariance matrix:

nlexpect(est, tcrossprod(as.matrix(c(x1-pt1,x2-pt2))))
x1 x2
x1 0.02609831 -0.122408
x2 -0.12240804 1.224730

End(No test)

1176a1184,1216

No test:

Non-linear test:

A simple one, what's the probability that product x1*x2 is between 0 and |E(x1)|?

nlexpect(est, x1x2 > 0 & x1x2 < abs(pt1), vectorize=TRUE, method='divonne')
x1
0.3914772

Then a more complicated one with the expected value of a polynomal in the coefficients

f <- function(x) c(poly=x[['x1']](6x[['x1']]-x[['x2']]^2))

This is the linearized test:

waldtest(est, f)['p.F']
p.F
0.7432511

In general, for a function f, the non-linear Wald test is something like

the following:

expected value of function

Ef <- nlexpect(est, f, coefs=c('x1','x2'))

point value of function

Pf <- f(c(pt1,pt2))

similar to a Wald test, but non-linear:

nlexpect(est, function(x) (f(x)-Ef)^2 > Pf^2, c('x1','x2'), vectorize=TRUE)
poly
0.6851808

one-sided

nlexpect(est, function(x) f(x)-Ef > abs(Pf), c('x1','x2'), vectorize=TRUE)
poly
0.267744

other sided

nlexpect(est, function(x) f(x)-Ef < -abs(Pf), c('x1','x2'), vectorize=TRUE)
poly
0.4193062

End(No test)

1219,1221c1259,1261
< id 0.93487358 0.01198024 -0.11085834
< firm 0.01198024 4.95403947 0.04691949
< foo -0.11085834 0.04691949 6.49011310

id 0.93372348 0.01224916 -0.11164451
firm 0.01224916 4.95014733 0.04605155
foo -0.11164451 0.04605155 6.48715940
1224,1226c1264,1266
< id 0.081184357 -0.004065798 -0.003258662
< firm -0.004065798 0.056924931 -0.003863477
< foo -0.003258662 -0.003863477 0.041169982


id 0.082334457 -0.004334716 -0.002472491
firm -0.004334716 0.060817078 -0.002995544
foo -0.002472491 -0.002995544 0.044123684

@lrdegeest
Copy link

My students also can't load lfe. Hope it's back up and running soon!

@ghost
Copy link

ghost commented Dec 5, 2020

@MatthieuStigler /tests/Examples/lfe-Ex.Rout.save corresponds to the output of the examples from the various functions' documentation

@MatthieuStigler
Copy link
Contributor

thanks @Helix123 My confusion is that I thought any x.Rout.save would have a corresponding x.R, and that R check would re-run x.R to compare the local x.Rout to the package x.Rout.save, but here I can't see here any lfe-Ex.R?

Or, in other terms, how would you re-run and update /tests/Examples/lfe-Ex.Rout.save?

@ghost
Copy link

ghost commented Dec 5, 2020

It is generated automatically by R CMD check (I think one level up the working directory). The official documentation seems to be very brief about it (1.1.5 in https://cran.r-project.org/doc/manuals/r-release/R-exts.html). Just place the file automatically generated in this specific directory/tests/Examples/ with the .Rout.save extension and with each run of R CMD check the examples' output is checked against this file.

Very useful but not well known!

@MatthieuStigler
Copy link
Contributor

Thanks @Helix123 . This is badly documented indeed. My understanding is that one needs to run R CMD CHECK, and extract manually the lfe-Ex.Rout file then copy into the package tests/examples, I could not find a way to generate it directly.

@MatthieuStigler
Copy link
Contributor

I've made a simple and blind update of update lfe-Ex.Rout.save, hopefully @sgaure can have a look at this soon!? :-)

@zeileis
Copy link

zeileis commented Dec 6, 2020

Thanks, Matthieu, much appreciated! As the CRAN task view "Econometrics" was affected by this, I asked the CRAN team what all the reasons for CRAN archival were so that you could help resolving them. They said that the issue was misuse of sprintf (seen in the differences from .Rout.save that you addressed now) and repeated non-response from the maintainer.

Note that tracking such issues and asking for updates is work for the CRAN team and they already did a non-maintainer update (NMU) in July for version 2.8-5.1. Hence the CRAN team was reluctant to become de facto maintainers for the package (they already had to do another NMU in March 2019) and decided to archive the package on CRAN. I can understand that decision and would not put the blame on them @SebKrantz.

Of course, @sgaure might also have his reasons for not responding to CRAN, it's a difficult year for everybody. But maybe passing on the maintenance might be an option that would help both Simen and the CRAN team?

Thanks to all of you for help with this!

One more comment: As far as I can see the changes made in the package by the CRAN team for 2.8-5.1 were also not ported to the sources here on GitHub, yet.

@ghost
Copy link

ghost commented Dec 6, 2020

Thank you for chiming in and for shedding some light on this, @zeileis!
(Please note that @SebKrantz is not the author of blog article you linked.)

@zeileis
Copy link

zeileis commented Dec 6, 2020

Ouch, touché! My sincere apologies @SebKrantz!! I overlooked the subtle difference between your name and that of the blog author. I should have been more careful about this, sorry!

@MatthieuStigler
Copy link
Contributor

Thanks Achim @zeileis, very useful! So we need to backport to git the update 2.8.5 and 2.8.5-1, I can look at that.

I am not sure what those sprintf errors were, all I did is to update manually the /tests/Examples/lfe-Ex.Rout.save.

Now apparently (from the last check) the main reason for rejecting the package was a note on Solaris: Compilation used the following non-portable flag(s): ‘-mt’ . Fixing that goes way beyond my skills, so we would need someone to help on this!? @Helix123 @lrdegeest @SebKrantz ?

One needs to look at lfe/configure, where this flag is mentioned multiple times?

ax_pthread_flags="pthreads none -Kthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config"

# -mt: Sun Workshop C (may only link SunOS threads [-lthread], but it
#      doesn't hurt to check since this sometimes defines pthreads and
#      -D_REENTRANT too), HP C (must be checked before -lpthread, which
#      is present but should not be used directly; and before -mthreads,
#      because the compiler interprets this as "-mt" + "-hreads")

@zeileis
Copy link

zeileis commented Dec 7, 2020

I asked about this, too, but my understanding of the response from the CRAN team is that the compiler flag was not the critical issue but rather the sprintf misuse + .Rout.save and the repeated non-response from the maintainer.

The compiler flag might be hard to avoid? Maybe it was like that for several releases already? I couldn't very this because there are no CRAN check archives for releases from 2019 or before. Provided the other issues are addressed one could reach out to the CRAN team and ask about this again.

In my view, the most critical aspect is to have a responsive maintainer for the package again - one way or another.

@MatthieuStigler
Copy link
Contributor

Ok, I submitted a cleaned version to CRAN, let us hope it goes through!

@zeileis
Copy link

zeileis commented Dec 7, 2020

Thanks! Let me know if I can re-include the package in the Econometrics task view - or if I can help with anything else.

@SebKrantz
Copy link
Author

SebKrantz commented Dec 7, 2020

Hey guys, thanks a lot for the effort ! As far as collapse is concerned. It will access higher-order centering functionality from fixest, which I was planning all along. Now laurent will make available the heterogenous slopes feature and provide more or less direct C++ access. The collapse operator HDW will allow to access this in an efficient lm-type formula interface, so you can do something like HDW(data, ~ id * poly(time, 3)) to project cubic or higher-order id-specific trends out of large datasets. Exciting stuff, even though it's sad that lfe has become unrealiable.

@MatthieuStigler
Copy link
Contributor

So my first attempt to submit the package hasn't worked out for now. CRAN maintainers are concerned about the Solaris compiler flag issue. They would eventually accept a submission now, under the promise that the issue would be solved quickly, which I definitely cannot commit to.

Would anyone be ready to have a look at this compiler issue? @SebKrantz you are quite versed in compiled code, right? ;-) Or @lrdegeest @Helix123 . Or @zeileis feel like discussing with the CRAN people?

Questions I have:

  1. The configure file is 3000 lines, I guess Simen didn't write it, where is it likely to come form (i.e. where is one likely to find help?)
  2. Can I simply disable the mt in line 3433 ax_pthread_flags="-mt,pthread pthread $ax_pthread_flags"? (two line above seems to be for solaris*)).

This is confusing as seems to be defined as: ax_pthread_flagsabove seems to already contain the -mt flag? ax_pthread_flags="pthreads none -Kthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config"

@ghost
Copy link

ghost commented Dec 8, 2020

Your effort is very much appreciated! Hope we can get there.

Sorry, no compiled code knowledge in R packages on my side (let alone make files, compiler flags and/or compiling for Solaris)

@SebKrantz
Copy link
Author

SebKrantz commented Dec 8, 2020

So this Makefile generating stuff is also hieroglyphs to me, but you can sort out things by googling and trying around. -mt as fas as I understand has something to do with multithreading. And it does not work anymore because Solaris has changed the default compiler. based on some googling i.e. here it seems to be an old flag, replaceable by -pthread (which is already there, so just try removing it). If that doesn't work, this documentation suggests one could replace -mt with '-D_REENTRANT -lthread' (as it is some form of shortcut). this post suggests we sould not use CFLAGS on Solaris, which seems to warrant some more substantial changes. First try removing and then replacing it. If none of these work, we may have to add some lines to that file that detect the operating system and remove -mt on solaris. As you already seem to have this set up @MatthieuStigler, I would be happy if you could try those things out. I'm not really smarter here. May also have to replace / remove lines 3608-3613.

@MatthieuStigler
Copy link
Contributor

Thanks @SebKrantz , this is very useful!

So good news is I believe the file is already doing some platform-specific checks, see lines in configure:

Line 3381: case $host_os in
Line 3423: solaris*)
Line 3433: ax_pthread_flags="-mt,pthread pthread $ax_pthread_flags"

And ax_pthread_flags seems to be defined as:
Line 3359: ax_pthread_flags="pthreads none -Kthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config"

That's a little confusing to understand what the end-result will be? Are those just concatenated? So we would have a line like:

-mt,pthread pthread pthreads none -Kthread -pthread -pthreads -mthreads pthread --thread-safe -mt pthread-config

That sounds very strange to me?

So far I just tried to remove the first -mt and just tested with

devtools::check_rhub(platform = c("solaris-x86-patched", "solaris-x86-patched-ods"),
                     interactive = FALSE)

This still had the issue... was I suppose to re-configure something myself? (beyond what devtools does?)? Or did I remove -mt in the wrong place?

Thanks!

@SebKrantz
Copy link
Author

Can you still try removing all the -mt's e.g. everywhere it appears in the repo (also in -mt, pthread), remove '-mt,'.

@MatthieuStigler
Copy link
Contributor

MatthieuStigler commented Dec 8, 2020

ok, I can try that.

But do I need to run anything locally, like ./configure? There's also a ax_pthread.m4 file, with first line

generated automatically by aclocal 1.15.1 -*- Autoconf -*-

which seems to copy those? Is that one created by configure?

@MatthieuStigler
Copy link
Contributor

I tried simply removing -mt (see commit on separate branch MatthieuStigler@47335b1), and unfortunately this seems to fail devtools::check_rhub(platform = c("solaris-x86-patched", "solaris-x86-patched-ods"))

Oracle Solaris 10, x86, 32 bit, R-release, Oracle Developer Studio 12.6

> est <- felm(y ~ x+x2|id+firm+foo, data=fr, keepX=TRUE)

 *** caught segfault ***
address 6, cause 'memory not mapped'

Oracle Solaris 10, x86, 32 bit, R-release

Note that some of these warnings might have happened anyway:

checking top-level files ... WARNING
  Output from running autoreconf:
  /opt/csw/share/aclocal/gtk.m4:7: warning: underquoted definition of AM_PATH_GTK
  /opt/csw/share/aclocal/gtk.m4:7:   run info Automake 'Extending aclocal'
  /opt/csw/share/aclocal/gtk.m4:7:   or see https://www.gnu.org/software/automake/manual/automake.html#Extending-aclocal


* checking compilation flags used ... NOTE
Compilation used the following non-portable flag(s):
  ‘-march=pentiumpro’

@waynelapierre
Copy link

It seems this package is no longer maintained anymore.

@MatthieuStigler
Copy link
Contributor

I just submitted a corrected version on CRAN two days ago! Hopefully the CRAN maintainers will accept uploading it even if there's still the compiler flag issue. So hopefully should be back on CRAN very
soon!

@grantmcdermott
Copy link
Contributor

While we wait for @MatthieuStigler's adopted version to hit CRAN, just an aside that I've written a little conversion package that takes lfe::felm code and translates it into the fixest::feols equivalent: https://github.com/grantmcdermott/lfe2fixest

@MatthieuStigler
Copy link
Contributor

MatthieuStigler commented Dec 16, 2020

I head back from the CRAN maintainers, who raised several concerns after doing a thorough check of the code. I am traveling today and starting a quarantine so will need a few days to fix the issues. In the meanwhile, if someone wants to try to look at one of the issues and send a pull request to my repo (https://github.com/MatthieuStigler/lfe), that would be welcome!

Concerns were:

  • \dontrun{} should only be used if the example really cannot be executed (e.g. because of missing additional software, missing API keys, ...) by the user. That's why wrapping examples in \dontrun{} adds the comment ("# Not run:") as a warning for the user.
    Does not seem necessary. Please unwrap the examples if they are executable in < 5 sec, or replace \dontrun{} with \donttest{}.
  • You are setting options(warn=-1) in your function. This is not allowed. Please rather use suppressWarnings() if really needed.
  • Please always make sure to reset to user's options(), working directory or par() after you changed it in examples and vignettes and demos.e.g.: tests old <- options(digits = 3) ... options(old)
  • Please do not modify the global environment (e.g. by using <<-) in your functions. This is not allowed by the CRAN policies.
  • Please ensure that you do not use more than 2 cores in your examples, vignettes, etc.

@grantmcdermott
Copy link
Contributor

grantmcdermott commented Dec 17, 2020

@MatthieuStigler I think that I'm the only other person here with write access to your fork. I've taken a quick stab at these and should have resolved everything — although, please check — except the second-to-last comment about <<-.

I can see two places where the <<- assignment is used: lines 1292--1295 of R/felm.R and lines 20--24 of R/condfstat.R. Replacing these would take a bit more thought, e.g. converting the existing functions to a while loop or similar. At any rate, I don't quite understand this comment. The way these operators are being used, they are simply redefining internal function values and never actually cause any variables to enter the users global environment. (Or perhaps, I have misunderstood?)

@MatthieuStigler
Copy link
Contributor

Thanks Grant!! I will check those! Still travelling

And no need to have direct write access to my repo, but would need to fork mine separately then do the pull reuqest

For the <<-, I will need to check, indeed it does not seem to export to global environment, I thought it always does. But yes it's not considered best practice, I'll try to see why it is required.

I see you have been re-building docs, and now exporting also doc fur unexported check_redundant_fe? Would you like it to be exported? There are still a few edge cases where I am not sure about redundancy of FEs...

@grantmcdermott
Copy link
Contributor

I see you have been re-building docs, and now exporting also doc fur unexported check_redundant_fe? Would you like it to be exported? There are still a few edge cases where I am not sure about redundancy of FEs...

Oh, sorry that was just an artefact of my doc checking/building. Agree that it still needs to be validated against edge cases so feel free to exclude.

@waynelapierre
Copy link

Any update?

@MatthieuStigler
Copy link
Contributor

The package is ready for (re-re-) submission, but unfortunately submissions to CRAN are not possible during the Winter Break, though they will resume on Jan 4th onwards.

@grantmcdermott thanks for all the good work! The .Rd problem was actually my fault, I omitted using `@noRd!

@eddelbuettel
Copy link

It just got back on per CRANberries:

image

@MatthieuStigler
Copy link
Contributor

I was just going to mention it @eddelbuettel ! To make up for spoiling the announcement, is there any chance you could have a quick look at the note on Solaris, Compilation used the following non-portable flag(s): ‘-mt’ to check if you see how to solve that issue? CRAN maintainers were reluctant to keep the package on CRAN with this message, and you are pretty much the only one I know who can solve that issue! ☺️

@eddelbuettel
Copy link

I don't know and have no access to Solaris either but glancing at this page I noticed you analysed configure. Don't do that. Look at configure.ac (sometimes called configure.in) which is the source governing the creation. Otherwise, maybe try r-package-devel and/or examine what other packages do?

yuvipanda added a commit to yuvipanda/datahub-old-fork that referenced this issue Mar 13, 2021
sgaure/lfe#41 seems to be
resolved, since the package is now on CRAN and can
be installed.

Fixes #2239
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

No branches or pull requests

7 participants