pds-data-dictionaries / PDS4-LDD-Issue-Repo

Issue repository for tracking all PDS4 Discipline Dictionary-related issues, new feature requests, and releases.
Apache License 2.0
2 stars 1 forks source link

[ldd-clipper] Add SUDA_Parameters class with attributes #262

Closed neese closed 3 months ago

neese commented 9 months ago

Issue Type This is an enhancement for the ldd-clipper mission dictionary.

Describe the issue identified (if applicable) This is the initial set of attributes for the SUDA instrument, to be integrated into a SUDA_Parameters class for use in SUDA product labels.

Describe the solution you'd like The attached document, provided by the SUDA team, includes a list of the attributes they would like included in the SUDA_Parameters class, along with units and descriptions. In addition, there is a sketch of the hierarchy for the desired attributes. This will likely require some massaging to get it into final form, in particular SUDA has requested an AID attribute, but we know the AID attribute will be included at the mission level. We're not sure of the best way to redesign the hierarchy to accommodate AID at the mission level.

SUDA team are open to suggestions for improvement of attribute names, descriptions, and hierarchy.

Describe alternatives you've considered N/A

LDD Dictionary Version N/A

PDS4 IM Version N/A

Need-by Date Fall 2023 for inclusion in the Nov. SUDA pre-launch peer review.

Additional context

Clipper_SUDA_LDD.docx

thareUSGS commented 8 months ago

@neese Thanks for the ping as I never saw this. So in the current version for the Clipper dictionary we have started with instrument sections like EIS_Parameters or ETHEMIS_Parameters. https://github.com/pds-data-dictionaries/ldd-clipper/blob/e7c52c7be2e4d9425f4ba0969c6c161a300978f9/src/PDS4_CLIPPER_IngestLDD.xml#L1077

This instead of the Class "Aid" what do think of "SUDA_Parameters" with "aid" as an attribute. Thus something like:

<clipper:SUDA_Parameters>
        <clipper:aid></clipper:aid>
    <clipper:event_count></clipper:event_count>
    <clipper:epoch></clipper.epoch>
    <clipper:start_time></clipper:start_time>
    <clipper:end_time></clipper:end_time>
        <clipper:SUDA_Event>
            <clipper:event_id></clipper:event_id>
        <clipper:event_time></clipper:event_time>
        <clipper:category></clipper:category>
        <clipper:tof_peak_count></clipper:tof_peak_count>
        <clipper:tof_mean></clipper:tof_mean>
        <clipper:tof_var></clipper:tof_var>
        <clipper:qv_max_amplitude><clipper:qv_max_amplitude>
        <clipper:qv_mean></clipper:qv_mean>
        <clipper:qv_var></clipper:qv_var>
        <clipper:qt_max_amplitude><clipper:qt_max_amplitude>
        <clipper:qt_mean></clipper:qt_mean>
        <clipper:qt_var></clipper:qt_var>
        <clipper:qi_max_amplitude><clipper:qi_max_amplitude>
        <clipper:qi_mean></clipper:qi_mean>
        <clipper:qi_var></clipper:qi_var>
       </clipper:SUDA_Event>
<clipper:SUDA_Parameters>
thareUSGS commented 8 months ago

@neese @seanhardman Nev branch released and built with 2 SUDA Classes and several attributes. I went ahead and created the structured as outlined above. Schemas are here for testing and reviewing: https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/bd223e011b08d8b329a6fa7b53836360bdf1c024

--updated link to fix a minor build issue

seanhardman commented 8 months ago

@thareUSGS, I believe @neese intended the aid element to reside in the _ObservationalInformation class as discussed at the last DAWG meeting. See #264 for the request to add that new element.

neese commented 8 months ago

@thareUSGS @seanhardman Thanks for this, I will provide it to the SUDA team for testing and we will discuss it in our SUDA tag-up this morning. When the aid element is implemented as intended we can test that too.

thareUSGS commented 8 months ago

@neese @seanhardman Thanks for the clarification for "aid". Moved it up under mission. I am still using the same SUDA structure (for review). New build to test: https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/8f8bcbbff063567ad59709d03f62fd1af84935e1

<Mission_Area>
  <Observation_Information>
     <clipper.mission_phase_name></clipper.mission_phase_name>
     ...
     <clipper:encouter_id></clipper:encouter_id>
     <clipper:aid></clipper:aid>
     ...

    <clipper:SUDA_Parameters>
        <clipper:event_count></clipper:event_count>
        <clipper:epoch></clipper.epoch>
        <clipper:start_time></clipper:start_time>
        <clipper:end_time></clipper:end_time>
            <clipper:SUDA_Event>
                <clipper:event_id></clipper:event_id>
            <clipper:event_time></clipper:event_time>
            <clipper:category></clipper:category>
            <clipper:tof_peak_count></clipper:tof_peak_count>
            <clipper:tof_mean></clipper:tof_mean>
            <clipper:tof_var></clipper:tof_var>
            <clipper:qv_max_amplitude><clipper:qv_max_amplitude>
            <clipper:qv_mean></clipper:qv_mean>
            <clipper:qv_var></clipper:qv_var>
            <clipper:qt_max_amplitude><clipper:qt_max_amplitude>
            <clipper:qt_mean></clipper:qt_mean>
            <clipper:qt_var></clipper:qt_var>
            <clipper:qi_max_amplitude><clipper:qi_max_amplitude>
            <clipper:qi_mean></clipper:qi_mean>
            <clipper:qi_var></clipper:qi_var>
           </clipper:SUDA_Event>
    </clipper:SUDA_Parameters>
  </Observation_Information>
</Mission_Area>
sapols commented 8 months ago

I'm finally getting around to looking at this in detail and wanted to note some issues I see in the current XSD file:

  1. It's missing two fields: epoch and tof_var.
  2. We should probably rethink the types of the SUDA_Events statistics fields (tof_peak_countqi_var):
    • They're all ASCII_Integer right now, but ASCII_Real makes more sense for fields like mean where even the mean of two ints can be a real.
    • We should allow some to be either ASCII_Integer OR ASCII_Real, like max_amplitude, where it depends on whether the data units are DN (ints; used in L1a) or physical (reals; used in L1b).
    • _(Kris and I are the ones who specified ASCII_Integer for these so that's our mistake.)_
  3. There are grammatical errors, like how "The class contain all the" is copy/pasted four times with the same mistake ("contains" is missing an 's').
  4. Nit-picky: but some <xs:documentation> entries do not start with capital letters while most do.
thareUSGS commented 8 months ago

I think I was able to get all the updates.

Note I added "aid" under Observation_Information and a different "aid" is also under SUDA_Parameters. Note it is the same "aid" attribute and definition, but they can be set independently.

new build for testing; https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/4b35348ac56d7234757059929a2929d6738cdc1d

thareUSGS commented 8 months ago

rebuild for 1.17 - 1.20 IM builds https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/b73939653d30276fbd1d875822df240ee2a01a86

sapols commented 7 months ago

Sorry @thareUSGS, you made these updates so quickly then I took so long to respond. We're having a delay on our end where I can't yet connect to the part of our database that has the values to fill in these new fields in our labels. So I won't know for sure if this is exactly what we'll want until the delay is resolved (hopefully soon).

That said, it looks good. Although I found one change that'd make sense and one I need to think about: (1) tof_peak_count should be ASCII_Integer, since it's a a count of peaks and fractional peaks aren't a thing. (2) epoch might make more sense as a string; but I'm not sure. It's the "Epoch defining the zero point of relative times," which I think for us is 1970-01-01 but I could be wrong. I'm not sure how I'd represent that date as an ASCII_Integer; but I'm also not sure about the best way to represent a date in general.

seanhardman commented 7 months ago

Hey @sapols, if you want to represent the Epoch as a date/time string, I would suggest using the _ASCII_Date_Time_DOYUTC or _ASCII_Date_Time_YMDUTC which would be represented as "YYYY-DDDThh:mm:ss.ssssssZ" or "YYYY-MM-DDThh:mm:ss.ssssssZ", respectively. Just as an aside, your Epoch may be "1970-01-01" but the Clipper SCLK Epoch is "2010-01-01T00:00:00.000Z".

sapols commented 7 months ago

Thanks @seanhardman. _ASCII_Date_Time_YMDUTC sounds good. I'll double check if we're using Clipper's 2010 epoch or something else.

thareUSGS commented 7 months ago

@sapols no problem, we are in a very fluid mode right now. We have time to iterate. I will set epoch as a ASCII_Date_Time_DOY_UTC and rebuild.

thareUSGS commented 7 months ago

new updates for

https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/af3291a7d076a9c16316bd266adbcd1fa0130a2b

no rush for any review.

sapols commented 7 months ago

@thareUSGS thanks. Any reason you made epoch _ASCII_Date_Time_DOYUTC instead of the _ASCII_Date_Time_YMDUTC I said sounds good? If there's a compelling reason to use DOY instead I'll use it, but the rest of the times in our labels are YMD which is why I said that.

thareUSGS commented 7 months ago

@sapols Just a copy/paste mistake. Thanks for catching. New build: https://github.com/pds-data-dictionaries/ldd-clipper/tree/v1.0.0.0_SUDA/build/development/c6b7fbb1d006f8b7f9c385b4e378996b9e8c1dd1

thareUSGS commented 4 months ago

@neese and @seanhardman Looks like @RayEspiritu is done with some final EIS update. There are 1.19.0.0 builds added back in also. I think we are ready for a release.

Currently, we would be releasing version 1.0.0.0 for Clipper dictionary. Does that sounds okay to all?

https://github.com/pds-data-dictionaries/ldd-clipper/tree/main/build (under dev*)

seanhardman commented 4 months ago

@thareUSGS I thought I heard you say at the last DAWG that there may be E-THEMIS updates coming. Did you want to wait for those before making the first release, or should we just go ahead and cut a release? I'm fine with doing a release now because it will simplify validation for all of these Peer Reviews.

neese commented 4 months ago

@thareUSGS @seanhardman @sapols Looks like SUDA is good to go with publishing the dictionary now. And it would simplify validation for the peer review.

thareUSGS commented 4 months ago

@seanhardman Yes there are ETHEMIS labels in review (pretty much passed so far). So I will take a quick scan to see what I can update.

sapols commented 4 months ago

@thareUSGS Sounds okay to me.

thareUSGS commented 3 months ago

v1000 release tagged. Will request that PDS Engineering hosts these tomorrow. https://github.com/pds-data-dictionaries/ldd-clipper/tree/main (right side, releases).