search

indica-mcf / Indica

Integrated DiagnostiC Analysis
GNU General Public License v3.0
6 stars 0 forks source link

Abstract reader assumes single quantity/array for "times" and "length" results #313

Closed pixelifytica closed 2 months ago

pixelifytica commented 6 months ago

Spotted a regression in AbstractReader whilst updating the PPFReader methods; it originally accepted different "times" and "length" results per quantity from the specific reader implementation, now has changed to a single result per instrument. This breaks anything with multiple sets of lines of sight in very different coordinates (e.g. JET bolometry and SXR with both vertical and horizontal arrays) where it doesn't make sense to use a single transform for the entire instrument.

The original implementation of {q}_times for time results but length = {q: [...]} for lengths didn't make sense to be different either, so we would need to standardise on something (in which case I'd lean towards the latter version).

This ties in to wanting to clarify the database results anyways, so @hakosaj and @marcosertoli what are your opinions on this? As part of #309 I'll "fix" AbstractReader to accept times and length as dictionaries, but lets discuss more to agree what to settle on properly!

pixelifytica commented 6 months ago

Clarify database_results dictionary expectation in abstract_reader and simplify reading implementation specific reader (e.g. st40reader, ppfreader)

marcosertoli commented 6 months ago

Hi Evie, no I would not do that change.

I've made it that way because each system must be treated separately. SXR H and V cameras are two different systems, so will have to be read separately.

pixelifytica commented 6 months ago

Ah I see the philosophy, and yep I can see how that simplifies things from the POV of the results dictionary. Ok I'll need to review which JET instruments have multiple systems gathered in that way (Bolometry, SXR and CXRS immediately come to mind, I don't think there are others) and roll that in with the general PPF updates in #309 .

marcosertoli commented 6 months ago

Great! You can check ST40 reader and look e.g. for the instruments SXRC_XY of which there are 2.

I see the reader method as being the method that will be used to read a specified system TYPE e.g. SXR camera, or TS or similar.

JET is a bit difficult because each system has strange, different database entries. But if you think of IMAS, identical system TYPES will have the same structure. We've done something similar, so all SXR systems have an identical structure, as do all BOLOMETERS or all LANGMUIR PROBES, etc.

But you have the IMPLEMENTATION QUANTITIES to fix that where needed.

Marco

From: Evie Litherland-Smith @.> Sent: Wednesday, March 27, 2024 10:18:43 AM To: indica-mcf/Indica @.> Cc: marcosertoli @.>; Assign @.> Subject: Re: [indica-mcf/Indica] Abstract reader assumes single quantity/array for "times" and "length" results (Issue #313)

Ah I see the philosophy, and yep I can see how that simplifies things from the POV of the results dictionary. Ok I'll need to review which JET instruments have multiple systems gathered in that way (Bolometry, SXR and CXRS immediately come to mind, I don't think there are others) and roll that in with the general PPF updates in #309https://github.com/indica-mcf/Indica/issues/309 .

— Reply to this email directly, view it on GitHubhttps://github.com/indica-mcf/Indica/issues/313#issuecomment-2022396689, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARZSM6HQYZEBZY22NGDDXDTY2KFHHAVCNFSM6AAAAABFKPZT36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRSGM4TMNRYHE. You are receiving this because you were assigned.Message ID: @.***>