mmeierer / REndo

REndo - A R package to control for endogeneity by using internal instrumental variable models
15 stars 4 forks source link

summary.copulaEndo #23

Closed Rgui closed 6 years ago

Rgui commented 8 years ago

Hi Patrik,

It has been a while... Could you please have a look at the syntax of the copulaEndo class. Does it make sense that the class declaration is S4 format but then the summary and coef methods are in S3? Will that be acceptable by CRAN?

Also, at this moment the summary.copulaEndo method does not function well. Could you have a look ? Thank you very much.

pschil commented 8 years ago

Hello Raluca, Some answers to your questions:

class declaration is S4 format but then the summary and coef methods are in S3?

This shouldn't be a problem in my view. The generics (summary, coef) are defined as S3 generics and I don't think you can / should change that. Simply implement the respective S3 function. (coef.liv, print.liv, ...) But you will have to make sure that your code in these functions will actually treat the object as an S4 object, ie use @ instead of $ and so on.

Will that be acceptable by CRAN

I guess so. They make a similar example like this in the documentation of ?methods.

However, throughout the code you are arbitrarily using and creating S3 and S4 objects of type "copulaEndo". I expect the problems of the summary function to be buried there. Also I find your usage of match.call rather ..... unconventional.

I would suggest to once discuss the expected package usage in person and then workout a code structure.

Rgui commented 8 years ago

Dear Patrik,

Thank you very much for taking time and looking over the code and giving me feedback.

Just let me know when you are next time in Zurich and have time to go over the code.

Best,

Raluca

On Fri, Sep 30, 2016 at 5:19 PM, pschil notifications@github.com wrote:

Hello Raluca, Some answers to your questions:

class declaration is S4 format but then the summary and coef methods are in S3?

This shouldn't be a problem in my view. The generics (summary, coef) are defined as S3 generics and I don't think you can / should change that. Simply implement the respective S3 function. (coef.liv, print.liv, ...) But you will have to make sure that your code in these functions will actually treat the object as an S4 object, ie use @ instead of $ and so on.

Will that be acceptable by CRAN

I guess so. They make a similar example like this in the documentation of ?methods.

However, throughout the code you are arbitrarily using and creating S3 and S4 objects of type "copulaEndo". I expect the problems of the summary function to be buried there. Also I find your usage of match.call rather ..... unconventional.

I would suggest to once discuss the expected package usage in person and then workout a code structure.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mmeierer/REndo/issues/23#issuecomment-250772440, or mute the thread https://github.com/notifications/unsubscribe-auth/AHWr6_ReBXiNgWdav9VYwbPq586dtzcVks5qvSiSgaJpZM4KIO-b .

Raluca Ioana Gui

PhD Student

University of Zurich | Department of Business Administration

Chair for Marketing and Market Research | URPP Social Networks

Andreasstrasse 15 | 8050 Zurich | Switzerland

Office AND 4.44

Phone: +41 44 634 9200 | Fax: +41 44 634 2940

raluca.gui@business.uzh.ch margot.loewenberg@business.uzh.ch

www.market-research.uzh.ch www.socialnetworks.uzh.ch