OHDSI Home | Forums | Wiki | Github

Breaking changes between 5.3.1 and 5.4

#1

@clairblacketer @all

today, discovered something that I was surprised to see (when ACHILLES broke) - the breaking change in the VISIT_* tables.

VISIT_OCCURRENCE

  • Admitting_source_concept_id → Admitted_from_concept_id
  • Admitting_source_value → Admitted_from_source_value
  • Discharge_to_concept_id → Discharged_to_concept_id
  • Discharge_to_source_value → Discharged_to_source_value

VISIT_DETAIL

  • Admitting_source_concept_id → Admitted_from_concept_id
  • Admitting_source_value → Admitted_from_source_value
  • Discharge_to_concept_id → Discharged_to_concept_id
  • Discharge_to_source_value → Discharged_to_source_value

Doesn’t it go against the whole semantic version idea to introduce these breaking changes into a minor version release? And why - this is just the naming, what real business benefit does it really carry to have us to go for it? I am sorry but what was the reasoning?

Thanks

Greg

#2

I think the position of the CDM team was that these fields were not widely used and they felt safe changing it.

I voiced my opinion that it is not the type of change you should allow in a minor CDM version, and described ways to alleviate backwards compatibility with these changes: add the additional column, leave the existing column, and populate both with the same data. It might have felt like data bloat to do it this way, but it keeps our tools from breaking.

I conveyed that message, and I think it was recieed.

They would be able to tell you, but I believe the ‘admitting’ vs. ‘admitted’ terminology was confusing in the meaning of the column (in the clinical sense of ‘admitting’ vs. ‘admitted from’. There’s a lot of code in libraries I have written (circe for example) where things like ‘primary events’ exist in the model when they are referred to as ‘cohort entry events’ when we talk about that data. It would make sense to change it, but it would break compatibility. I personally have a zero-tolerance (although WebAPI is guilty and you may be surprised which devs violated it), but I can understand why it happens. All that being said, I’ve begged that those types of changes are not in minor releases.

1 Like
#3

thank you, @Chris_Knoll - I am 100% with you on this one. The standards are a set of rules to be followed and not created to be broken when the first opportunity arise. Since we said that we follow the semantic versioning here - i kind of relaxed and assumed no breaking changes. So probably a few other folks. While I do understand that the original naming was - let’s say - incorrect, but that means that we just need to be more carefully reviewing the proposals next time.

t