Bug fix for chmAddPCA - Githubissues

marohrdanz commented 1 month ago

This pull request is a bug fix for issue #67.

Some additional checking of input arguments was also added--for example to check that the third argument is a prcomp object and has prcomp$x.

Additionally, the existing error checking messages were tweaked to hopefully be slightly easier for users to quickly understand. Example:

If first argument to chmAddPCA is not an NG-CHM, the original code gave this error:

Error in chmAddPCA();
    is(hm, "ngchmVersion2") is not TRUE

I tweaked this slightly to give message:

Error in chmAddPCA():
    First argument (hm) must be an ngchmVersion2 object.

ChrisWakefield commented 1 month ago

Is the 'ndim' argument supposed to be the required number of dimensions, as this change implies, or is it the max number desired, in which case the functionality should not be changed?

bmbroom commented 1 month ago

Is the 'ndim' argument supposed to be the required number of dimensions, as this change implies, or is it the max number desired, in which case the functionality should not be changed?

It should be the max number desired. If the user specifies 5 dims but there are only 2, we go with 2.

marohrdanz commented 1 month ago

My instinct on ndim is that if the user requested more than available in the prc input data, then probably they made an error somewere and we should let them know. Is that too much handholding?

ChrisWakefield commented 1 month ago

Imagine instead that the arg was named 'maxdim' rather than 'ndim'. I think that is as bmbroom described it.

marohrdanz commented 1 month ago

I pushed three additional commits to address the issues mentioned above:

Use better class checking for first argument (926f4dc)
Make ndim what it was before: the max desired number of dimensions (afb577c)
Tweak the error message if axis arg isn't a single character (56274fc)

I'm hoping there's time at today's meeting to discuss these further, and additional changes requested.

bmbroom commented 1 month ago

Sounds good to me.

I am still on PTO so cannot be there today. Go ahead and merge if everyone else is okay with it.

Bradley

On Tue, Oct 15, 2024, 10:32 AM Mary Rohrdanz @.***> wrote:

I pushed three additional commits to address the issues mentioned above:

Use better class checking for first argument (926f4dc https://github.com/MD-Anderson-Bioinformatics/NGCHM-R/commit/926f4dc1646cfefa510c68810ffcc7daed11b0c4 )

Make ndim what it was before: the max desired number of dimensions ( afb577c https://github.com/MD-Anderson-Bioinformatics/NGCHM-R/commit/afb577cb0cf8c363321316dfdede5d00264e1f3c )

Tweak the error message if axis arg isn't a single character (56274fc https://github.com/MD-Anderson-Bioinformatics/NGCHM-R/commit/56274fcc87a034151099dece4ba610e237ae8b2b )

I'm hoping there's time at today's meeting to discuss these further, and additional changes requested.

— Reply to this email directly, view it on GitHub https://github.com/MD-Anderson-Bioinformatics/NGCHM-R/pull/68#issuecomment-2414104503, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAI76BRF2MODEUHM32FEUNTZ3URQBAVCNFSM6AAAAABPXUEFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJUGEYDINJQGM . You are receiving this because your review was requested.Message ID: @.***>

MD-Anderson-Bioinformatics / NGCHM-R

Bug fix for chmAddPCA #68