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

Visit_detail_concept_id #145

Closed
gowthamrao opened this issue Jan 6, 2018 · 9 comments
Closed

Visit_detail_concept_id #145

gowthamrao opened this issue Jan 6, 2018 · 9 comments

Comments

@gowthamrao
Copy link
Member

gowthamrao commented Jan 6, 2018

visit_detail_concept_id INTEGER NOT NULL ,

Are we using visit_detail_concept_id or visit_concept_id for visit_detail table? There is a clash between documentation and DDL for this field https://github.com/OHDSI/CommonDataModel/blob/master/Documentation/CommonDataModel_Wiki_Files/StandardizedClinicalDataTables/VISIT_DETAIL.md
Documentation says it is visit_concept_id, although visit_detail_concept_id is less ambiguous, but visit_concept_id was proposed to the CDM workgroup as part of the visit_detail proposal.

I like the visit_detail_concept_id, to avoid ambiguity - but the concept_id is still belongs to the 'Visit' domain (not 'Visit detail' domain). If we decide to go with visit_detail_concept_id, then we will also need to use visit_detail_source_concept_id. Current implementation is visit_source_concept_id

visit_source_concept_id INTEGER NULL ,

@clairblacketer
Copy link
Contributor

I vote we keep it as visit_detail_concept_id. It can contain the same values as visit.visit_concept_id but it makes the clear distinction that this is the concept as it pertains to the detail row, not the visit overall. I put a note to discuss this at tomorrow's workgroup meeting.

@gowthamrao
Copy link
Member Author

Thank you. I am fine with both, but then we will need to change visit_source_concept_id to visit_detail_source_concept_id. How about visit_detail_type_concept_id etc

@rtmill
Copy link

rtmill commented Jan 9, 2018

In response to the conversation today on the WG call:

It seems to me there is a common convention in the CDM to avoid using the duplicate field names in different tables unless the field is a foreign key to another clinical data table. For instance, for any reference to person the foreign key is 'person_id', for any reference to care site the foreign key is 'care_site_id', etc, no matter the table.

If, however, the field isn't a key to another table (e.g. x_date) or references a vocabulary table (e.g. x_concept_id) then the field name is unique across all tables. observation_concept_id vs. procedure_concept_id, observation_date vs procedure date, etc.

I believe the benefit of keeping this convention is two-fold:
-Clarity on definitions of concepts and mappings (can use concept class/domain to specify field)
-Avoidance of errors caused by ambiguity of variable names of joins

Anyway, my suggestion is to avoid using any of the same field names as the visit_occurrence table unless its a foreign key to the visit_occurrence table itself. visit_detail_concept_id or detail_concept_id or finding another name for a visit_detail, which to me is more of a short visit (encounter?) than a fact about a visit.

@gowthamrao
Copy link
Member Author

Thank you @rtmill and @clairblacketer - i agree with changing the names (makes sense), and everything is < 30.

Old field name New field name Length
visit_concept_id visit_detail_concept_id 23
visit_source_concept_id visit_detail_source_concept_id 30
visit_type_concept_id visit_detail_type_concept_id 28
visit_source_value visit_detail_source_value 25
visit_start_datetime visit_detail_start_date 23
visit_start_datetime visit_detail_end_date 21
visit_end_date visit_detaiil_start_datetime 28
visit_end_datetime visit_detail_end_datetime 25
visit_source_value visit_detail_source_value 25

@hripcsa
Copy link

hripcsa commented Jan 9, 2018 via email

@rtmill
Copy link

rtmill commented Jan 9, 2018

So I've been doing some thinking... (this is never good)

What if instead we get rid of the visit_detail table altogether and just add an optional parent_visit_id field to the visit_occurrence table? That's the only difference in the tables right? If visit_detail is capable of being a parent visit to another visit_detail record (relations can be hierarchical or sequential according to wiki), then really the two tables are functionally equivalent. If you are only looking for what we are currently defining as visit_occurrences you could just take the visits that have parent_visit_id as NULL.

Reasoning:
-avoids this naming issue
-avoids redundancy
-fields in both tables reference same set of concepts
-doesn't require addition of new field to each clinical data table that has a FK to visits (visit_detail_id). Any clinical data (condition, obs, meas, etc) could reference a visit, micro-visit, encounter, or whatever the most specific granularity you have
-adding an optional parent_visit_id to visit_occurrence makes the two tables equivalent (I think?)

I would guess that I'm missing something?

@gowthamrao
Copy link
Member Author

@rtmill yes, we went thru this in the CDM WG. Should we have a long table that self-references itself. Lot of reasons were expressed to not to want to do this; including confusion, technical challenges etc

@parisni
Copy link

parisni commented Jan 31, 2018

Also as statued there #153
there is a need to add two columns:

  column to add
admitting_concept_id
discharge_to_source_concept_id

@clairblacketer
Copy link
Contributor

closing - this was addressed in v5.3.2 though #153 is still open

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

No branches or pull requests

5 participants