search

RNO-G / mattak

RNO-G dataformats
1 stars 1 forks source link

Refactor voltage calibration #44

Closed fschlueter closed 1 day ago

fschlueter commented 6 months ago

If you are fine with that I will also add a _ at the end or front of every member variable.

fschlueter commented 6 months ago

We should make sure that we do not change the name of things stored in the root files!

martin01021207 commented 6 months ago

We should make sure that we do not change the name of things stored in the root files!

@fschlueter These changes look fine, but reading existed calibration constant files might become an issue since one of the tree branch names has changed. But I guess it's fine, I will just have to produce calibration constant files out of bias scans after this PR is merged.

fschlueter commented 6 months ago

We should make sure that we do not change the name of things stored in the root files!

@fschlueter These changes look fine, but reading existed calibration constant files might become an issue since one of the tree branch names has changed. But I guess it's fine, I will just have to produce calibration constant files out of bias scans after this PR is merged.

Hm, yeah - not ideal but I guess okay when worth is. Are you both fine with the coding style? I would also add a pre or post fix to the member variables. @cozzyd, this class is never getting serialized right? So no danger of losing backwards compatibility?

fschlueter commented 6 months ago

I was also thinking of renaming all function to use a capital letter at the beginning. But this seemed quite some work..

fschlueter commented 1 week ago

@cozzyd it seems I only renamed some variables here. But I am worried that this might break backwards compatibility if I rename member variables. I can of course test it but can you say already that this would go down south?

fschlueter commented 1 week ago

@martin01021207 I reverted the changes of the object names to be stored in the files. Thanks for catching that!

fschlueter commented 1 week ago

hm I am puzzled by the error msg

cozzyd commented 1 week ago

looks like either the file is corrupt or some random bug was introduced in uproot?

martin01021207 commented 1 week ago

@martin01021207 I reverted the changes of the object names to be stored in the files. Thanks for catching that!

@fschlueter If we haven't produced a big number of calibration files yet I guess you can still change the name to what you think it's better to read.

fschlueter commented 1 day ago

looks like either the file is corrupt or some random bug was introduced in uproot?

But why it is not failing on the other branch?

fschlueter commented 1 day ago

I just went a head and merged it. Lets see if it still fails on the main branch now. I might revert the merge than

fschlueter commented 1 day ago

arg the target branch was a different one

fschlueter commented 1 day ago

This was falsely merged into a wrong branch. But it contained commits which should not have been merged anyway.... So lets abandon this plan.