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

Fix exportToSQLite errors caused by old table name and parameter order #159

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

mccullen
Copy link
Contributor

I was having issues with the exportToSQLite function and had to make these changes to get it to work. I assume the issue was that this function is using an old CDM table name (cohort_attribute?) and some parameter orders have changed. Either that or I'm not doing something right on my end.

@burrowse
Copy link
Collaborator

burrowse commented Dec 8, 2023

@mccullen Thanks so much for submitting this PR! The parameter order makes sense. The table name issues are a combination of typos and this function originally supporting version only OMOP v5.3. The list of tables you provided support OMOP v5.4 which is great but we'd want to add some logic (like we do in other functions here when defining the event_tables variable to support both versions of the OMOP data model. Would you be able to edit this in the PR? Alternatively, we can add this as an update on our side. Thanks again for your input!

@mccullen
Copy link
Contributor Author

mccullen commented Feb 7, 2024

@mccullen Thanks so much for submitting this PR! The parameter order makes sense. The table name issues are a combination of typos and this function originally supporting version only OMOP v5.3. The list of tables you provided support OMOP v5.4 which is great but we'd want to add some logic (like we do in other functions here when defining the event_tables variable to support both versions of the OMOP data model. Would you be able to edit this in the PR? Alternatively, we can add this as an update on our side. Thanks again for your input!

Oh, sorry for the late reply. Sure, I will have a look when I get the chance and make those updates.

Copy link
Collaborator

@burrowse burrowse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you for this contribution @mccullen !

@burrowse burrowse merged commit c298e20 into OHDSI:main Feb 23, 2024
1 of 2 checks passed
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 this pull request may close these issues.

2 participants