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

This check the output file name not the data value in the data frame #126

Closed
djhbs opened this issue May 17, 2023 · 5 comments · Fixed by #208
Closed

This check the output file name not the data value in the data frame #126

djhbs opened this issue May 17, 2023 · 5 comments · Fixed by #208
Assignees

Comments

@djhbs
Copy link

djhbs commented May 17, 2023

https://github.com/atorus-research/xportr/blob/231e959b84aa0f1e71113c85332de33a827e650a/R/write.R#LL33C1-L33C82
Could you please confrim this check refer to the output file name not the data value?
For the file name, if the DM_F can be used when the strict_checks = FALSE,and the DM_F means the full SDTM that contains the parent DM and SUPPDM
Thank you very much.

@djhbs
Copy link
Author

djhbs commented May 22, 2023

I realised that it can be renamed after output by file.rename

@djhbs djhbs closed this as completed May 22, 2023
@djhbs djhbs reopened this May 31, 2023
@djhbs
Copy link
Author

djhbs commented May 31, 2023

The file.rename not change the internal sas dataset name when opened by SAS universal viewer.
The following is I create a temperary tmp.xpt then rename
image

@EeethB EeethB self-assigned this Nov 14, 2023
@bms63 bms63 assigned averissimo and unassigned EeethB Dec 5, 2023
@cpiraux
Copy link
Collaborator

cpiraux commented Dec 7, 2023

I have investigated this issue.

https://github.com/atorus-research/xportr/blob/231e959b84aa0f1e71113c85332de33a827e650a/R/write.R#LL33C1-L33C82
Could you please confrim this check refer to the output file name not the data value?
For the file name, if the DM_F can be used when the strict_checks = FALSE,and the DM_F means the full SDTM that contains the parent DM and SUPPDM
Thank you very much.

The check is performed on the filename used to name the XPT file. This name is a metadata in the xpt and it is not updated when the XPT filename is changed.

name <- tools::file_path_sans_ext(basename(path))

if (stringr::str_detect(name, "[^a-zA-Z0-9]")) {
  abort("`.df` cannot contain any non-ASCII, symbol or underscore characters.")
}

write_xpt(data, path = path, version = 5, name = name)

The underscores are allowed in xpt but it was not allowed in FDA sdTCG. I checked the latest version of the FDA sdTCG and it is not clear if underscores are allowed or not.

FDA sdTCG Version March 2018
image

FDA sdTCG Version October 2023
image

I tested the conversion from XPT to SAS7BDAT with SAS, and I observed that the name of the dataset is derived from the metadata name, not the file name. If the filename is renamed, the SAS dataset is not renamed.

I think using the filename as the name of the xpt is a good approach. This is also requested in the FDA sdTCG .
image

I don’t think we can do anything regarding the change of the xpt name when the filename is renamed.

Something to check, if underscores are allowed or not. And, update the name check accordingly in xportr_write().

@averissimo averissimo assigned cpiraux and unassigned averissimo Dec 7, 2023
@bms63
Copy link
Collaborator

bms63 commented Dec 14, 2023

@cpiraux So we can close this issue as out of scope and make an additional issue regarding underscores?

@djhbs
Copy link
Author

djhbs commented Dec 14, 2023

@cpiraux Thank yor for detailed explanation. my suggestion is if we can add strict_checks = TRUE as defual, but the file name can be use the "_" when strict_checks = FALSE.

@cpiraux cpiraux linked a pull request Dec 18, 2023 that will close this issue
14 tasks
cpiraux added a commit that referenced this issue Dec 19, 2023
bms63 added a commit that referenced this issue Jan 16, 2024
Closes #126 - Update check for filename to allow underscore
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

Successfully merging a pull request may close this issue.

5 participants