legend-exp / LegendGeSim.jl

High-level Julia package for LEGEND HPGe detector simulation
Other
2 stars 8 forks source link

Update IO to new LEGEND metadata format #21

Closed fhagemann closed 1 year ago

fhagemann commented 1 year ago

There were some format changes to the metadata files in https://github.com/legend-exp/legend-detectors/pull/5/ which cause the current version of LegendGeSim.jl to throw errors when reading files in the new format.

I updated the IO of LEGEND metadata files by incorporating the following changes:

@sagitta42 : could you check if this update works for you?

fhagemann commented 1 year ago

I wouldn't be surprised if legend-testdata also needs an update to the new format in order to test the new version of the package with the new metadata format.

oschulz commented 1 year ago

@sagitta42 , could you take a look at this?

fhagemann commented 1 year ago

The last commit fixes how the inner taper is subtracted. Before the commit, we would subtract more than needed, resulting in there being a gap between the contact an the semiconductor. This is fixed now

Before

image

After

image

oschulz commented 1 year ago

Should I merge this now, @fhagemann ?

fhagemann commented 1 year ago

I would prefer if someone would first cross check and maybe update legend-testdata to properly test the changes

sagitta42 commented 1 year ago

Hi everyone, sorry for not responding, my mental health took quite a plunge. Will check the update now

sagitta42 commented 1 year ago

Oh yeah I remember now, I went into complete halt because after our Munich meeting, I implemented reading impurity profile from metadata and converting to SSD/siggen, and turned out Lukas did that as well, without asking me, and even though I was implementing it right there in Munich, asking him questions etc... So I had to resolve all that and figure out in what way I'd keep that, what from what he did is the same etc... I'll just scratch it all and keep whatever is there now, to resolve this issue first, and come back to it later...

oschulz commented 1 year ago

So we can merge this @sagitta42 ?

sagitta42 commented 1 year ago

@fhagemann @oschulz I updated the testdata json example to follow the new format. That should fix the not successful checks here I guess?

fhagemann commented 1 year ago

@oschulz could you re-run the tests?

oschulz commented 1 year ago

@oschulz could you re-run the tests?

Uh, I think they did run on the current state of the PR?

fhagemann commented 1 year ago

@oschulz could you re-run the tests?

Uh, I think they did run on the current state of the PR?

Sure, but the testdata has changed in the mean time, and I would like to see if the tests pass now 😅

sagitta42 commented 1 year ago

@oschulz yeah I don't think it was re-run just now somehow automatically, don't think it's smart enough to detect a change in test data and run itself again ahah

fhagemann commented 1 year ago

The tests might not run on the new LEGEND testdata, as the package LegendTestData.jl to provide the test data still references an old commit 4de6dd2 => https://github.com/legend-exp/legend-testdata/commit/4de6dd2154ba437e7c3eba525f609a4ace7f29b1

@oschulz Could you update this to the latest commit of legend-testdata: a210811?

sagitta42 commented 1 year ago

There will be another update to metadata: tapering will be expressed in terms of height and radius rather than height and angle. The arguments in favor of this are: 1) this is how the vendor states it, so want to just keep the original info, 2) using angle introduces imprecision. That means this should also be updated in LegendGeSim...

fhagemann commented 1 year ago

There will be another update to metadata: tapering will be expressed in terms of height and radius rather than height and angle. The arguments in favor of this are: 1) this is how the vendor states it, so want to just keep the original info, 2) using angle introduces imprecision. That means this should also be updated in LegendGeSim...

@sagitta42 LegendGeSim should be able to handle both (see https://github.com/legend-exp/LegendGeSim.jl/blob/main/src/legend_detector_to_ssd.jl#L43-L53), so I don’t think we need to update anything here

oschulz commented 1 year ago

@oschulz Could you update this to the latest commit of legend-testdata

Sure: legend-exp/LegendTestData.jl#2

oschulz commented 1 year ago

@oschulz Could you update this to the latest commit of legend-testdata

Just released LegendTestData.jl v0.2.0

fhagemann commented 1 year ago

Lets try and see if the tests pass now 🤞

fhagemann commented 1 year ago

Tests are passing, any objections from your side before merging this? @oschulz @sagitta42

sagitta42 commented 1 year ago

no objections on my side

oschulz commented 1 year ago

Fine with me!