Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize Sofast measurement HDF format #131

Merged

Conversation

braden6521
Copy link
Collaborator

@braden6521 braden6521 commented Jul 3, 2024

Standardized the format which a Sofast measurement is saved in an HDF5 file by making the following edits. This will help reduce future confusion by standardizing all Sofast measurement files.

  • Updated the associated Sofast measurement classes to save in and load from only the current save format.
  • Updated all Sofast HDF5 measurement files in the unit test data.
  • Updated missing/incorrect documentation throughout Sofast measurement classes.

@braden6521 braden6521 self-assigned this Jul 3, 2024
Copy link
Collaborator

@bbean23 bbean23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for standardizing this interface! Please either:

A) Update the documentation to reflect my comment and re-submit for approval.
B) Merge as-is, but create a follow-up ticket to clarify the documentation.

@@ -30,14 +30,14 @@ def save_to_hdf(self, file: str, prefix: str) -> None:
Prefix to append to folder path within HDF file (folders must be separated by "/").
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear whether the intention is for the prefix parameter to be a folder (ending in '/') or if it is just supposed to be the beginning of the name. I'd suggest updating the documentation and/or parameter name with the intended usage. Maybe something like:

def save_to_hdf(self, file: str, prefix: str = '') -> None:
    """Saves data to given file. Data is stored as: PREFIX + Folder/Field_1
    Parameters
    ----------
    file : str
        HDF file to save to
    prefix : str, optional
        Prefix to prepend to the folder path within the HDF file. Suggested use
        is as a prefix to the folder name such as "202407_BCS_", so that folders
        can still be easily machine identified. Note that folders must be
        separated by "/", so if prefix is a folder name, then it should end in
        "/". Default is empty string ''.
    """

Note also that I changed "append" to "prepend".

@braden6521 braden6521 merged commit 796417b into sandialabs:develop Aug 9, 2024
4 checks passed
@braden6521 braden6521 deleted the unified_sofast_measurement branch August 9, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants