-
Notifications
You must be signed in to change notification settings - Fork 12
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
Tranlation bug for SDDSRVYR
?
#83
Comments
If there's only a single category then there's not much purpose for translation. In some cases the translation string is a long sentence, so I wanted the ability to suppress translation when it's not necessary. There's always the option of using nhanesTranslate to translate |
I can see how this particular field is important when combining across cycles. Looping in Robert to get his opinion on how to handle. |
Right, I came across this while trying to combine the DEMO tables. This leads to an inconsistency between the standard and the docker version of I think whatever we decide, I don't know what the default of So perhaps one thing to change would be the following: define number of categories as the number of rows in the codebook, not number of unique values in the data. Would that make sense? That still doesn't answer what the default value of |
Hi,
Yes, I can see where this is going to be a problem. So, I agree that
the minimum change needed is
to accept and forward arguments.
While I see the concern you have, Chris, about the single value and long
sentences, I wonder if the simplicity of just doing
the conversion no matter what makes it easier to document and to ensure
that the code works. Otherwise, I worry that there
will be a continuing string of special cases that we would need to document.
That I think is your call as you have managed the package for some time
and know the users better than I do. My vote is
typically for simplicity of maintenance - mostly because that is the most
challenging work....
Robert
…On Sat, Feb 3, 2024 at 11:27 AM Deepayan Sarkar ***@***.***> wrote:
Right, I came across this while trying to combine the DEMO tables. This
leads to an inconsistency between the standard and the docker version of
nhanes(). And while the code for the first 10 cycles are 1, 2, ..., 10,
of course for the pre-pandemic cycle it is 66 for some reason.
I think whatever we decide, nhanes() should accept and forward arguments
to nhanesTranslate().
I don't know what the default of mincategories should be. I can't think
of any other situation where it's meaningful to have only one value. But
actually there are 2 possible values here (if we include . = Missing), and
it so happens that there are no missing values in the data. I can see the
opposite situation arising --- all values are missing --- in which case
translation will also be disabled, which is not going to be a good thing.
We have to check whether that actually happens anywhere.
So perhaps one thing to change would be the following: define number of
categories as the number of rows in the codebook, not number of unique
values in the data. Would that make sense?
That still doesn't answer what the default value of mincategories should
be, but at least takes care of this particular problem.
—
Reply to this email directly, view it on GitHub
<#83 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC7TWA6HJIJVLNX6VEXVWELYRZQOTAVCNFSM6AAAAABCWMU2BOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRVGM3TINZSGE>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
--
Robert Gentleman
***@***.***
|
We have
Yet,
SDDSRVYR
is not translated bynhanes()
:I think this is due to the default of
mincategories = 2
innhanesTranslate()
(there are no missing values, and so only one unique value):https://github.com/cjendres1/nhanes/blob/master/R/nhanes_translate.R#L52
I don't think this is sensible behavior. Is there any particular reason for the default not to be 1? My suggestion would be to change the default to 1 otherwise.
Also, currently there is no way to specify this when calling
nhanes()
, which makes this effectively hard-coded. There should be a provision to pass this on tonhanesTranslate()
fromnhanes()
.The text was updated successfully, but these errors were encountered: