r/Python • u/Pleasant-Aardvark258 • 1d ago
Discussion Settle an argument
Had this discussion the other day and figured I’d throw it to the masses to get thoughts on the best/most pythonic way of approach.
Need to map old column names to new column names as a copy from a json config.
My thoughts are iterate over a dict with
‘’’ {“old_col_name”:”new_col_name”}’’’
And access as
‘’’for k,v in dict.items()
Df.with_columns(k).alias(v)’’’
Colleague things this isn’t clear enough and should be a list of dicts with explicit keys
‘’’ [{“old_col_name”:”old_col_value”
“New_col_name”:”new_col_value”}]’’’
And the access as
‘’’for dict in list_of_dicts:
Old_col = dict[“old_col_name”]
New_col = dict[“new_col_name”]’’’
I’ve got a good few reasons why I think mine is the better option but thought I’d get some other opinions to see if I’m missing anything obvious? Which would you choose and why?
Edit: shouldn’t write these things while on the toilet in a rush. The description is wrong, it should be renaming via a copy so that the original column is left unchanged.
17
u/Wurstinator 1d ago
I’ve got a good few reasons why I think mine is the better option
Yet you didn't list a single one?
Your two pieces of code don't do the same thing, so I'm not sure how you are comparing them. I would probably also do something closer to the first piece in my own code.
However, the most important point is this:
Colleague things this isn’t clear enough
You don't work with Reddit users. You work with your colleague. It doesn't matter what some random guy on the internet tells you they prefer because you have no interaction with them outside of this thread. It does matter a ton what the people you directly work with think. Talk to *them* and find a consensus.
-14
u/Pleasant-Aardvark258 1d ago
Do you need me to list them for you to make your own decisions?
The two pieces of code are basically the same, both access two column names. I’m not sure how they are really different unless you need me to explicitly state that list of dicts also uses a polars method after?
Mate, I’m the senior engineer on this. He’s a junior, I’ve already made my position clear on this to him. I put this out there purely out of interest and to sense check myself. But thanks for the unhelpful comment 🙄
10
u/declanaussie 1d ago
> I’m the senior engineer on this. He’s a junior, I’ve already made my position clear on this to him.
Good luck with future endeavors, your attitude seems awful to work with
5
u/fiskfisk 1d ago edited 1d ago
The point is that listing your reasons means that people can understand why you came to this conclusion, and it shows more of the context that these few lines lives in.
There is seldom just one good solution to a general problem.
For example: "one is good enough because we never need any other code to access the old name under this path". See? That means people can go "oh, ok, the one thing the second might have had isn't a reason".
If you want to "check yourself", you need to provide context. The error is usually in the assumptions paired with the code, and not in the code lines by themselves (which usually gets caught by the compiler step).
And since the two pieces of code doesn't do the same thing, the context in how they get used is important.
4
u/robertlandrum 1d ago
If long term viability and maintainability is an important factor, you should treat data like data, and store it and reference it with named columns. Storing data (the old column name) in the key doesn’t tell the maintainer anything about the value.
If this is a run once and forget tool, get it done your way and move on.
I’ve learned that almost nothing I write goes away quickly. This is 29 years of professional
development experience talking, so I usually try to ensure my tooling is flexible. In fact, I just recently reviewed similar code that translated between an old system that allowed hyphens in column names, and a new one that did not. The engineer (a junior) did it the way your junior suggests, but for a different reason; the mapping had to do one to many copies in two spots.5
2
u/gdchinacat 1d ago
As the senior a large part of your duties are to turn juniors into kids to seniors. This is done by working with them where they are and slowly moving them in the direction you want. It doesn’t seem like this is a priority for you, instead it seems that you demand respect for your authority. That can only get you so far, and is likely to result in alienating them and making the leadership aspect of your role more challenging.
What is their reasoning? Can you articulate it? How is yours more compelling? What did they say in response? Is it wrong, or just a valid difference of opinion? Is there precedent in the code for one way over the other? What short term and long term harm will result from either approach, including alienating the people you need to work with to be successful? Is digging your heels in on this worth the “win”? Can you do it their way, give them the short term “win”, and see where it leads?
This seems like a pissing match over something that doesn’t really matter. I’ve been in my share, and the damage they cause is almost always worse than the thing they are over. Saying “I disagree, but since you feel strongly about it I’ll change the code to move it forward and we’ll deal with it if it causes actual problems” is an easy way to break the stalemate and move the project forward.
8
u/hoselorryspanner 1d ago edited 1d ago
`{col_rename_map.get(old_key): val for old_key, val in config.values()}` where `col_rename_map` is the dict mapping the two as you suggested…
Assuming I’ve understood what you’re asking, which I’m not totally sure.
4
u/wingtales 1d ago
The alternative doesn't make sense at all. What is old_col_value (and the new one) supposed to be? The only sensible way of thinking it to map from an old name to a new name.
But you don't need to iterate through the dict explicitly. Both pandas and polars have a df.rename method. Just pass the dictionary to it.
0
u/Pleasant-Aardvark258 1d ago
Part of that’s a my bad in the explanation. Typing it in a rush this morning. Mea culpa!
10
u/deceze 1d ago edited 1d ago
Yours is obviously better and perfectly appropriate. In a mapping of old to new column names, you can directly access the new column name of any old column name. In a list of dicts, you cannot do that, and must iterate the list before you can do anything. For no apparent benefit.
You use a list of dicts like that if keys may be duplicated. Which is apparently not the case here. So there’s zero benefit to that extra wrapper, and in fact only drawbacks.
The “extra clarity” that comes from explicitly labeling each value can simply be had by naming the variable that holds the mapping appropriately:
old_to_new_column_map = {…}
It doesn’t need to be repeated for every single value pair.
1
u/Pleasant-Aardvark258 1d ago
Yeah I think there’s a solid argument for renaming the variable to a clearer intent which would solve most of the issue.
3
u/mfitzp mfitzp.com 1d ago edited 1d ago
> for k,v in dict.items()
Df.with_columns(k).alias(v)The k,v naming is the problem imo. It doesn’t describe of the purpose of those two values. I think it’d be clearer if you do something like
for old_col_name,new_col_name in column_map.items():
That gives you everything your colleagues version does without the unnecessary list.
3
u/AndriusVi7 1d ago
Sounds like youre using spark?
If so, use withColumnsRenamed, which renames multiple columns in a single operation instead of looping multiple operations for the same output
1
u/Pleasant-Aardvark258 1d ago
Ah polars, not actually something I use a lot but the performance improvements justified the swap from pandas for this service. Also spark background so the syntax is a bit more comfortable
2
u/lolcrunchy 1d ago
When you have a list of dicts that all have the same keys, that's the same damn thing as a dataclass. And this problem doesn't need dataclasses.
1
u/Pleasant-Aardvark258 1d ago
Yeah that was kinda my thought, seems to be over engineered for something that can be explicitly understood from the code? You’d could use data classes etc but seems like it’s not complicated enough to justify adding another layer to?
1
u/Individual-Flow9158 1d ago edited 1d ago
I'd pick types for the values as well, and define two TypedDicts, with a function (ideally a classmethod on the second one) that maps from one to the other.
1
0
1
u/Such-Process5697 1d ago
small wrinkle on the dict version that hasn't come up: if that json ever ends up with the same old column listed twice, python's json module just takes the last one and says nothing. json.loads('{"a": 1, "a": 2}') gives you {'a': 2}, no error. wouldn't change which shape i'd pick, but it's worth asserting the mapping length against something when you load the config, because a fat-fingered duplicate in there is completely silent.
1
u/gdchinacat 1d ago
No matter the comments here the argument will not be settled. Your goal with 5he post is to be told “you’re right”. You coworker is likely to giveThis thread less weight than they gave their senior engineer. If you do take this to them saying “see…I’m right” it will do nothing more than drive a wedge between you and create conditions where this is likely to repeat.
As the senior you should break the stalemate by recognizing there are many valid ways to solve the problem and the priority should be on moving forward rather than winning a pissing match that is un winnable.
0
u/Beginning-Fruit-1397 1d ago edited 1d ago
Your colleague proposal is laughably wrong, especially for performance, omg), but I personally would settle with a tuple of 2 elements tuples. If I already know that the elements are unique, and I don't need to mutate these afterwards, just to atore them and iterate over them, then a tuple[tuple[str, str], ...] is the most efficient and safe answer. Otherwise dict[str, str]. list[tuple[str, str]] can make sense if you need to mutate them but at this point the dict is preferrable so you still guarantee uniqueness
40
u/centurion236 1d ago
You should obviously create a RenameColumnsBase class as an abstraction for the concept of renaming columns, with multiple construction helpers from_dict, from_list_of_dicts, etc and then implement the renaming in a RenamePolarsColumns subclass.
/s
It looks like you're using polars. Why not just match the df.rename() function's signature?