API: Render axes attributes as list of double quoted strings #155

carterbox · 2021-11-03T20:44:36Z

Previously, arrays of strings or byte arrays were truncated to only show the first element, but this is incompatible with the Nexus standard which expects axes to be provided as an array of strings.

This PR changes the behavior of utils.decode_byte_string() and h5tree.renderAttribute so that all types of strings are converted to strings and arrays of strings are converted to lists of strings. This allows rendering of strings and lists of strings with double quotes. However, it also causes some knock-on effects where unrelated functions which always expected a string, now need to deal with lists of strings. These cases which caused existing tests to fail were patched.

Closes #129

Previously, arrays of strings or byte arrays were truncated to only show the first element, but this is incompatible with the Nexus standard which expects axes to be provided as an array of strings. Closes prjemian#129

punx/tests/test_issue129.py

prjemian · 2021-11-10T22:57:36Z

Actually, this is not quite what I had in mind. More like (with this file created):

import h5py

with h5py.File("/tmp/demo.h5", "w") as h5:
    nxentry = h5.create_group("Scan")
    nxentry.attrs["NX_class"] = "NXentry"

    nxdata = nxentry.create_group("data")
    nxdata.attrs["NX_class"] = "NXdata"
    nxdata.attrs["axes"] = "two_theta"
    nxdata.attrs["signal"] = "counts"
    nxdata.attrs["two_theta_indices"] = 0
    nxdata.attrs["multi_str"] = "one two three".split()

    ds = nxentry.create_dataset(
        "counts",
        data=[
            1037, 1318, 1704, 2857, 4516
        ]
    )
    ds.attrs["units"] = "counts"

    ds = nxentry.create_dataset(
        "two_theta",
        data=[
            17.92608, 17.92591, 17.92575, 17.92558, 17.92541
        ]
    )
    ds.attrs["units"] = "degrees"

Should give a report like this:

(bluesky_2022_1) prjemian@zap:~/.../prjemian/punx$ punx tree /tmp/demo.h5 

!!! WARNING: this program is not ready for distribution.

/tmp/demo.h5 : NeXus data file
  Scan:NXentry
    @NX_class = NXentry
    counts:NX_INT64[5] = [1037, 1318, 1704, 2857, 4516]
      @units = counts
    two_theta:NX_FLOAT64[5] = [17.92608, 17.92591, 17.92575, 17.92558, 17.92541]
      @units = degrees
    data:nxdata
      @NX_class = NXdata
      @axes = two_theta
      @multi_str = ["one", "two", "three"]
      @signal = counts
      @two_theta_indices = 0

Today, the code renders this one attribute: @multi_str = one (it only gets the first element in the list)

Thinking that all string attributes should be rendered with double quotes and lists of strings should be rendered with square brackets.

prjemian · 2021-11-10T23:03:01Z

more like this:

(bluesky_2022_1) prjemian@zap:~/.../prjemian/punx$ punx tree /tmp/demo.h5 

!!! WARNING: this program is not ready for distribution.

/tmp/demo.h5 : NeXus data file
  Scan:NXentry
    @NX_class = "NXentry"
    counts:NX_INT64[5] = [1037, 1318, 1704, 2857, 4516]
      @units = "counts"
    two_theta:NX_FLOAT64[5] = [17.92608, 17.92591, 17.92575, 17.92558, 17.92541]
      @units = "degrees"
    data:nxdata
      @NX_class = "NXdata"
      @axes = "two_theta"
      @multi_str = ["one", "two", "three"]
      @signal = "counts"
      @two_theta_indices = 0

…ring

carterbox · 2021-11-11T01:50:36Z

I changed the behavior of decode_byte_string because I don't think anyone would use numpy.array(["this is a test"]). When they should be doing numpy.array(["this", "is", "a", "test"]). Is that OK?

prjemian

@carterbox This looks good. Are there any other test cases to construct? If not, merge when ready.

carterbox · 2021-11-11T19:21:17Z

I don't know why the CI passes. There are a few failing tests for me locally!

prjemian · 2021-11-11T19:28:05Z

Please copy the complete the console failure report here.

Some attributes were being rendered not using the _renderAttributes function. Now this function has been refactored, so that all attribute rendering occurs in the same code segment.

carterbox · 2021-11-11T22:30:03Z

/home/dching/Documents/punx/punx/data/Data_Q.h5 : NeXus data file
  sasentry01:NXentry
    @NX_class = ["NXentry"]
    @canSAS_class = ["SASentry"]
    end_time:NX_CHAR = b'2014-04-25T09:34:06.087'
    start_time:NX_CHAR = b'2014-04-25T09:33:43.637'
    sasdata01:NXdata
      @I_axes = ["Q,Q"]
      @NX_class = ["NXdata"]
      @Q_indices = ["0,1"]
      @canSAS_class = ["SASdata"]
      @canSAS_version = ["0.1"]
      @signal = ["I"]
      I:NX_FLOAT32[100,100] = __array
        __array = [
            [0.0, 0.0, 0.0, '...', 0.0]
            [0.0, 0.0, 0.0, '...', 0.0]
            [0.0, 0.0, 0.0, '...', 0.0]
            ...
            [0.0, 0.0, 0.0, '...', 0.0]
          ]
        @IGORWaveType = 2
        @axes = ["Q"]
        @signal = [1]
        @uncertainty = [""]
      Q:NX_FLOAT32[100,100] = __array
        __array = [
            [0.46678692, 0.45850673, 0.45038438, '...', 0.45968014]
            [0.45850673, 0.4500742, 0.44179693, '...', 0.45126957]
            [0.45038444, 0.44179687, 0.43336153, '...', 0.4430146]
            ...
            [0.45968014, 0.45126957, 0.4430146, '...', 0.45246175]
          ]
        @IGORWaveType = 2

This example file for the validation tests is now "wrong" because it uses a numpy array of a single string. What should I do about that?

carterbox · 2021-11-11T22:31:27Z

The validate tests are currently failing where they expect a string but get a list instead. For example:

punx/validate.py:239: in validate
    self.build_address_catalog()
punx/validate.py:269: in build_address_catalog
    self._group_address_catalog_(None, self.h5)
punx/validate.py:297: in _group_address_catalog_
    self._group_address_catalog_(parent, group[item])
punx/validate.py:293: in _group_address_catalog_
    obj = get_subject(parent, group)
punx/validate.py:283: in get_subject
    v = ValidationItem(parent, o)
punx/validate.py:409: in __init__
    self.classpath = self.determine_NeXus_classpath()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <punx.validate.ValidationItem object at 0x7f062d75f700>

    def determine_NeXus_classpath(self):
        """
        determine the NeXus class path
    
        :see: http://download.nexusformat.org/sphinx/preface.html#class-path-specification
    
        EXAMPLE
    
        Given this NeXus data file structure::
    
            /
                entry: NXentry
                    data: NXdata
                        @signal = data
                        data: NX_NUMBER
    
        For the "signal" attribute of this HDF5 address: ``/entry/data``,
        its NeXus class path is: ``/NXentry/NXdata@signal``
    
        The ``@signal`` attribute has the value of ``data`` which means
        that the local field named ``data`` is the plottable data.
    
        The HDF5 address of the plottable data is: ``/entry/data/data``,
        its NeXus class path is: ``/NXentry/NXdata/data``
        """
        if self.name == SLASH:
            return ""
        else:
            h5_obj = self.h5_object
    
            classpath = str(self.parent.classpath)
            if classpath == CLASSPATH_OF_NON_NEXUS_CONTENT:
                logger.log(INFORMATIVE, "%s is not NeXus content", h5_obj.name)
                return CLASSPATH_OF_NON_NEXUS_CONTENT
    
            if not classpath.endswith(SLASH):
                if utils.isHdf5Group(h5_obj):
                    if "NX_class" in h5_obj.attrs:
                        nx_class = utils.decode_byte_string(h5_obj.attrs["NX_class"])
>                       if nx_class.startswith("NX"):
E                       AttributeError: 'list' object has no attribute 'startswith'

punx/validate.py:486: AttributeError

Fails because nx_class is ["NX"] instead of "NX"

prjemian · 2021-11-12T16:16:40Z

I agree that the examples are incorrect. They can be used to test for invalid structures. Will the proper structure be caught by other tests? If so, then the test can be modified to check that invalid structure is detected.

It is possible that other tests and data files may need such review.

…ring

carterbox · 2021-11-12T19:27:24Z

unitests aren't being run on this PR. It's because I'm using my own fork instead of using branches here. Should I add:

on: [push, pull_request]

to the GitHub action?

prjemian · 2021-11-13T21:11:43Z

Should I add:

Sure.

punx/tests/test_issue129.py

prjemian

Changes look fine to me.

prjemian · 2021-11-13T22:35:07Z

The test says it is testing a list of axes but the only list attribute is a different one. I'll modify and upload.

…

On Sat, Nov 13, 2021, 4:31 PM Daniel Ching ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In punx/tests/test_issue129.py <#155 (comment)>: > + +def test_render_multiple_axes_attribute(hfile): + """Ensure axes attributes are rendered as list of double quoted strings. + + @axes should be saved as an array of byte strings or an array of object + strings because the NeXus standard says so. Single strings separated by + whitespace or other charachters will not be rendered correctly. + """ + assert os.path.exists(hfile) + with h5py.File(hfile, "w") as h5: + nxentry = h5.create_group("Scan") + nxentry.attrs["NX_class"] = "NXentry" + + nxdata = nxentry.create_group("data") + nxdata.attrs["NX_class"] = "NXdata" + nxdata.attrs["axes"] = "two_theta" You want me to only compare that one line in the report to the reference instead of all lines in the report? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARMUMF7ONPEKPN2DOKTDOLUL3RMBANCNFSM5HJ4WXZA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

prjemian · 2021-11-14T16:42:24Z

The point is that @axes in the existing test is not a list of str. The test must construct and test a test data file with two or more independent axes and a @signal with commensurate dimensions. 2-D (or higher) data. For this, need to consult the NeXus documentation and construct the data. Also need to confirm the data type of the @axes attribute, whether str or list(str).

prjemian · 2021-11-14T17:00:19Z

In the NeXus documentation (in the Index), the @axes attribute can be added either to the NXdata group (preferred), or to a dataset (discouraged, this is the old way, still valid but discouraged).

The 2-D example shows the structure is list(str) as shown:

@axes=["time","pressure"]

where the datasets time and pressure exist in the same group.

The unit test should create a data file with the structure of the 2-D example and assert the structure and content of the @axes attribute and also verify that the named axes exist as datasets in the same group.

prjemian · 2021-11-14T17:02:37Z

Random numbers for the actual data (of the datasets) are sufficient. These values will not be tested.

prjemian · 2021-11-15T17:38:48Z

Removed the print() calls since they do not contribute to the tests and will not appear in the pytest console output.

prjemian · 2021-11-15T17:48:33Z

Number data types are OS-dependent as shown in this example when testing on Windows 10:

        # compare line-by-line, except for file name
        # TODO: data size is OS-dependent?
        for ref, xture in zip(report[1:], structure[1:]):
>           assert ref.strip() == xture.strip()
E           AssertionError: assert 'data:NX_INT32[4,3] = [ ... ]' == 'data:NX_INT64[4,3] = [ ... ]'
E             - data:NX_INT64[4,3] = [ ... ]
E             ?            ^^
E             + data:NX_INT32[4,3] = [ ... ]
E             ?            ^^

punx\tests\test_issue129.py:77: AssertionError

prjemian · 2021-11-15T18:27:27Z

@carterbox This is ready for your comment. Should I recycle my review approval?

carterbox

LGTM

carterbox added 3 commits November 3, 2021 16:00

API: Render axes attributes as list of double quoted strings

d6815a6

Previously, arrays of strings or byte arrays were truncated to only show the first element, but this is incompatible with the Nexus standard which expects axes to be provided as an array of strings. Closes prjemian#129

STY: Break lines at 80 chars

7d170c4

DEV: Used wrong issue number in test name

81ea1ce

carterbox force-pushed the 128-double-quote-string branch from 2e4efd4 to 81ea1ce Compare November 3, 2021 21:01

BUG: Decode bytestring axes too

b152ff2

carterbox commented Nov 3, 2021

View reviewed changes

punx/tests/test_issue129.py Outdated Show resolved Hide resolved

STY: Add a whitespace

a6e3a10

prjemian assigned carterbox Nov 5, 2021

prjemian added the task label Nov 5, 2021

prjemian added this to the v1.0.0-release milestone Nov 5, 2021

carterbox added 2 commits November 5, 2021 12:32

DEV: Make hdf5 string conversions clearer

09a6eeb

Merge branch 'main' into 128-double-quote-string

736ecc1

carterbox requested a review from prjemian November 7, 2021 21:55

prjemian and others added 3 commits November 10, 2021 17:32

TST prjemian#128 proposed test

51de4da

REF: Wrap ALL string attributes in dquotes

078b033

Merge remote-tracking branch 'upstream/main' into 128-double-quote-st…

6e72f5f

…ring

prjemian approved these changes Nov 11, 2021

View reviewed changes

carterbox added 3 commits November 11, 2021 14:48

DOC: Clarify the role of decode_byte_string

b0cf193

REF: Use renderSingleAttribute everywhere

e7a3e3c

Some attributes were being rendered not using the _renderAttributes function. Now this function has been refactored, so that all attribute rendering occurs in the same code segment.

TST: Combine issue 129 tests into one file

142c678

carterbox added 5 commits November 12, 2021 13:02

BUG: Handle non-string NXClass attributes

138244b

BUG: Handle non-string NXDL specifications

c81d3ac

TST: Update scores for validation tests

3bba649

STY: Add missing indent

7744f62

Merge remote-tracking branch 'upstream/main' into 128-double-quote-st…

b2020c4

…ring

carterbox requested a review from prjemian November 12, 2021 19:27

prjemian reviewed Nov 13, 2021

View reviewed changes

punx/tests/test_issue129.py Outdated Show resolved Hide resolved

prjemian reviewed Nov 13, 2021

View reviewed changes

CI: Run unit tests on PRs too

0989dff

prjemian added 5 commits November 14, 2021 11:10

MNT prjemian#155

2d1a667

TST prjemian#155 test axes attribute for 2-D case

dd8d62c

TST prjemian#155 refactor second test

c615050

TST prjemian#155 refactor 3rd test

a0f3679

CI prjemian#155 flake8

32d48a8

TST prjemian#155 ignore these data values and types

f81c51d

TST: add byte strings to string conversion test

e9aca7b

carterbox commented Nov 15, 2021

View reviewed changes

carterbox merged commit 7875832 into prjemian:main Nov 15, 2021

carterbox deleted the 128-double-quote-string branch November 15, 2021 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Render axes attributes as list of double quoted strings #155

API: Render axes attributes as list of double quoted strings #155

carterbox commented Nov 3, 2021 •

edited

Loading

prjemian commented Nov 10, 2021 •

edited

Loading

prjemian commented Nov 10, 2021 •

edited

Loading

carterbox commented Nov 11, 2021

prjemian left a comment

carterbox commented Nov 11, 2021

prjemian commented Nov 11, 2021

carterbox commented Nov 11, 2021

carterbox commented Nov 11, 2021 •

edited

Loading

prjemian commented Nov 12, 2021

carterbox commented Nov 12, 2021 •

edited

Loading

prjemian commented Nov 13, 2021

prjemian left a comment

prjemian commented Nov 13, 2021 via email

prjemian commented Nov 14, 2021 •

edited

Loading

prjemian commented Nov 14, 2021 •

edited

Loading

prjemian commented Nov 14, 2021

prjemian commented Nov 15, 2021

prjemian commented Nov 15, 2021

prjemian commented Nov 15, 2021

carterbox left a comment

API: Render axes attributes as list of double quoted strings #155

API: Render axes attributes as list of double quoted strings #155

Conversation

carterbox commented Nov 3, 2021 • edited Loading

prjemian commented Nov 10, 2021 • edited Loading

prjemian commented Nov 10, 2021 • edited Loading

carterbox commented Nov 11, 2021

prjemian left a comment

Choose a reason for hiding this comment

carterbox commented Nov 11, 2021

prjemian commented Nov 11, 2021

carterbox commented Nov 11, 2021

carterbox commented Nov 11, 2021 • edited Loading

prjemian commented Nov 12, 2021

carterbox commented Nov 12, 2021 • edited Loading

prjemian commented Nov 13, 2021

prjemian left a comment

Choose a reason for hiding this comment

prjemian commented Nov 13, 2021 via email

prjemian commented Nov 14, 2021 • edited Loading

prjemian commented Nov 14, 2021 • edited Loading

prjemian commented Nov 14, 2021

prjemian commented Nov 15, 2021

prjemian commented Nov 15, 2021

prjemian commented Nov 15, 2021

carterbox left a comment

Choose a reason for hiding this comment

carterbox commented Nov 3, 2021 •

edited

Loading

prjemian commented Nov 10, 2021 •

edited

Loading

prjemian commented Nov 10, 2021 •

edited

Loading

carterbox commented Nov 11, 2021 •

edited

Loading

carterbox commented Nov 12, 2021 •

edited

Loading

prjemian commented Nov 14, 2021 •

edited

Loading

prjemian commented Nov 14, 2021 •

edited

Loading