Rename column strat#488
Conversation
psnairne
left a comment
psnairne
left a comment
There was a problem hiding this comment.
I think the only real issue is how the strategy interacts with Multi/Regex Identifiers.
Like I say below, I'm not convinced this strategy is worth the effort, but it's up to you!
| /// The table | ||
| /// | ||
| /// ```csv | ||
| /// PatientId, conditions, dob |
There was a problem hiding this comment.
I think it might be more useful if you gave an example that renamed a column name like "little interest or pleasure in doing things" to "HP:1234567". In other words, the example you have in mind.
Then you could add a comment somewhere that this should be used when a column has HpoInHeader context (or in general when the header is relevant for collection, if you want to be less specific)
| &self, | ||
| tables: &mut [&mut ContextualizedDataFrame], | ||
| ) -> Result<(), StrategyError> { | ||
| for (old, new) in self.renaming.iter() { |
There was a problem hiding this comment.
(old_header,new_header) would be more helpful)
| let (new_sc, old_id) = if let Some(old_sc) = table.get_sc_by_col_name(old) { | ||
| ( | ||
| SeriesContext::new( | ||
| Identifier::Single(new.to_string()), |
There was a problem hiding this comment.
What if the identifier of the old_sc was Multi or Regex? The Multi situation, can be fixed easily, the Regex situation can't.
Suppose we have a bunch of columns all with HpoInHeader context, covered by a Regex (in fact this was one of the use-cases of this context right?). If for whatever reason you need to rename one of them, then your current code would mean the rest of them wouldn't be collected.
My personal feeling, is that column renaming should be done before PhenoXtract, because of difficulties like this, and because each strategy we make requires upkeep
There was a problem hiding this comment.
I don't think the same applies to the MultiContextSplitter strategy you were describing to me earlier, but IMO column names can be easily changed by the user if they need to be. Especially since it's the same amount of effort to write out the aliases in the config, as it is just to rename the columns in the original table
There was a problem hiding this comment.
If you have strong feelings about this though, the strategy can stay since it doesn't affect the rest of the code, but maybe the documentation can be reworded to reflect how it interacts with Single/Multi etc. Maybe you just need to say "only do this for columns identified by an SC with Identifier of single type"
| let names: HashSet<&String> = renaming.values().collect(); | ||
| if names.len() < renaming.len() { | ||
| return Err(ConstructionError::DuplicateColumnNames { | ||
| reason: "Configured ColumnRenamingStrategy does feature duplicates." |
There was a problem hiding this comment.
should be "Configured ColumnRenamingStrategy has duplicate column names"
| } | ||
|
|
||
| #[rstest] | ||
| fn test_empty_renaming_map_is_noop() { |
2 participants
No description provided.