First round of changes proposed by mission team

cgobat commented 8 months ago

Summary

This PR mostly contains the changes that were discussed at SBN's last migration meeting on Feb 1. Commit messages should hopefully provide a fairly clear picture of what was updated, but to summarize:

Adds a new nh:Engineering_Conversion_Coeffcients class and associated attributes for handling cases where polynomial coefficients are currently provided in the text description of table columns in the PDS3 labels. For example: swa_0340156832_0x586_sci.lbl (Ctrl+F search for "coefficients")
Updates the nh:MVIC_Conversion_Constants class name to nh:Radiometric_Conversion_Constants, since other instruments besides MVIC use the same set of conversion constants
Adds a more complete set of permissible detector names and types within the nh:Detector/nh:detector_name and nh:Detector/nh:detector_type attributes
- Also adds a nh:SWAP_Details (to mirror the existing nh:MVIC_Details) class as a child of nh:Detector to hold a nh:sweep_samples_count attribute
Adds an optional nh:arrokoth_constant attribute to nh:Resolved_Source and nh:Unresolved_Source
Adds a couple more permissible nh:mission_phase_names: "KEM1 Cruise" and "KEM2 Cruise"

Test Data and/or Report

All tests passing after updates.

Related Issues

Blocks #12.

acraugh commented 6 months ago

Every other value is spelled out except for "CCD", which seems to be well-known , but I would actually prefer to see spelled out as well for consistency and clarity that will better stand up over time. These labels are here to support future users, not the mission team, after all. Including both is also a fine option, as in "Charge-Coupled Device (CCD)". Explicit is good.

Are these being copied verbatim from some source, or is it relatively easy to make the change to the spelled-out value? I view it as highly desirable, but at some point practicality enters the equation.

-Anne.

On Thu, Mar 28, 2024 at 6:19 PM Kate Crombie @.***> wrote:

@.**** commented on this pull request.

On src/PDS4_NH_IngestLDD.xml https://github.com/pds-data-dictionaries/ldd-nh/pull/11#discussion_r1543789380 :

I had to look up the SSD definition, and had the same thought as Anne did, but the instrument team used "SSD" . Not really helpful, I know. Best I can do for now is say that as we get additional feedback from the teams we will update definitions.

— Reply to this email directly, view it on GitHub https://github.com/pds-data-dictionaries/ldd-nh/pull/11#discussion_r1543789380, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBV2QD6EBH4KWPRMEIAMMDY2SJQ3AVCNFSM6AAAAABC445F4GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRXGQ3DENRTGY . You are receiving this because your review was requested.Message ID: @.***>

acraugh commented 6 months ago

I'd like to merge this PR (#11) this morning, if possible. The doc and regression tests can wait for cleanup. The big question is this: Can I change the Permissible_Values "CCD" to "Charge-Coupled Device (CCD)" and "SSD" to "Solid State Silicon Detector (SSD)"? I would note that "SSD" commonly stands for "Solid State Drive" (as opposed to "HDD") and has for some years, so if this is something other than an SSD, and it sounds like it is, then it definitely needs words rather than letters alone.

Will this cause a major disruption on your end, @cgobat?

cgobat commented 6 months ago

Nope, both those changes work for me. Thanks!

pds-data-dictionaries / ldd-nh