Match replace lazytree branch names #225
Conversation
|
I think we should try not to make and I still think we should have a |
|
I thought it's minimal enough ;) the problem I see with the rename is that it requires a two step process and also carrying the information to two separate places while it could easily be done in one go. If we do not need to do a lot of copying, we can also go for the rename option. I'll check that out. At least in my case, this rename thing is pretty normal and needed almost everywhere 🙈 |
|
Another problem is that the user needs to know the normalised branch beforehand to create the rename map. This also means that changing the branch name normalisation is a breaking change for these awkward brach names, where you expect the user to do renaming. |
eh, find another similar package that has the "main function" (defined as the one that most user will use to get the table-like thing back) handle renaming, I checked a few, and they don't usually do this: |
|
Hm, I think I have to contradict, they do: Also just because others don't do it, it does not mean it could make sense in our case 🙂 ROOT is of course much more different compared to CSV and handling complex, nested structures is way more complicated. Just to regurgitate my example from above t = LazyTree(f, "E", [r"Evt/trks/trks.(dir|pos).([xyz])")will currently translate to the fieldnames while the original class which was serialised to this sub-branch is an So I have to deal with the renaming of dozens of fields to make it more "usable" in the code and for the user. If the renaming need to happen after the generation of these awkward names, one really needs to implement this by hand and rely on the normalisation procedure, which I consider to be something like an implementation detail (more or less). To mitigate this, the function to normalise branch names would need to be exported, in my opinion. The code would need to be hardcoded if the renaming is external to t = LazyTree(f, "E", [r"Evt/trks/trks.(dir|pos).([xyz])")
new_t = rename(t,
["Evt_trkstrksdirx" => "dir_x",
"Evt_trkstrksdiry" => "dir_y",
"Evt_trkstrksdirz" => "dir_z",
"Evt_trkstrksposx" => "pos_x",
"Evt_trkstrksposy" => "pos_y",
"Evt_trkstrksposz" => "pos_z",]
)compared to this single line, where the information is minimal and it renames multiple branches in one go. t = LazyTree(f, "E", [r"Evt/trks/trks.(dir|pos).([xyz])" => s"\1_\2"])Of course, Apropos CSV. Taking your example with Maybe my cases are more like edge cases and other users of Anyways, let me know what you think. |
|
OK, I need some feedback here, I could not fix #226 yet but I need to move on now and leave that for later. My design process now heavily depends on whether we want this replacement feature or not 😉 |
|
Sorry, forgot to ping @Moelf |
|
I'm not against but please make sure CI passes and there's example and documentation |
|
OK thanks for the go! Of course I'll make everything pass and remove all the debug stuff (it's still draft) so that you can put your final stamp on it 😄 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
==========================================
- Coverage 90.08% 88.21% -1.88%
==========================================
Files 18 18
Lines 2129 2121 -8
==========================================
- Hits 1918 1871 -47
- Misses 211 250 +39
... and 5 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
tamasgal
force-pushed
the
match-replace-lazytree
branch
from
b1fce13 to
6115035
Compare
|
Tests pass on the latest |
|
looks good to me. we don't care nightly error I think. |
The idea is to give the user the ability to rename branches. The current implementation of @Moelf is amazing, but sometimes the branch paths are long and annoying and result in awkward columns.
Before this patch:
After this patch: