feat: Show path and ID of missing node when trying to find channel in…

[soyers]

Description

This PR proposes to add more information to some error messages that re related to missing nodes in the XML metadata.
In the context of pylibczirw it is possible to provoke undefined states where the image metadata does not match the actual subblocks. For example, consider this code snippet:

  with create_czi("test.czi")) as test_czi:
      # Write data to the CZI since behavior on empty images in not well-defined
      test_czi.write(data=np.zeros((100, 100), dtype=np.uint8), plane={"C": 0})
      ### THIS LINE IS ACTUALLY NEEDED test_czi.write(data=np.zeros((100, 100), dtype=np.uint8), plane={"C": 1})
      # Write metadata
      test_czi.write_metadata(
          document_name="TestWriteMetadata",
          scale_x=1.0,
          scale_y=2.0,
          scale_z=3.0,
          channel_names={0: "TestCh0", 1: "TestCh1"},
          display_settings={
                0: ChannelDisplaySettingsDataClass(
                    True,
                    TintingMode.Color,
                    Rgb8Color(np.uint8(0x01), np.uint8(0x02), np.uint8(0x03)),
                    0.2,
                    0.8,
                ),
                1: ChannelDisplaySettingsDataClass(
                    True,
                    TintingMode.Color,
                    Rgb8Color(np.uint8(0xFF), np.uint8(0xFE), np.uint8(0xFD)),
                    0.3,
                    0.7,
                ),
            },
      )

Note that the image is supposed to yield two channels (as defined in the display settings and the chanel names), but there is only written data for one channel since test_czi.write(data=np.zeros((100, 100), dtype=np.uint8), plane={"C": 1}) is commented.
Since libczi is fetching the channel ID and name from the metadata that is being written when writing the subblocks, it will, as expected, not be able to fetch the channel ID and name and rais an exception when writing the metadata. However, the error being raised is very generic and does not provide any information about what the underlying cause. This PR proposes to enable the error to show the path and index of the node such that the underlying issue is easier to understand.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• [x ] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Run the code snippet above with pylibczirw baced by the PR version of libczi.

Checklist:

• [x ] I followed the Contributing Guidelines.
• [x ] I did a self-review.
• [x ] I commented my code, particularly in hard-to-understand areas.
• I updated the documentation.
• I updated the version of libCZI following Versioning of libCZI depending on the type of change
  • Bug fix -> PATCH
  • New feature -> MINOR
  • Breaking change -> MAJOR
• I have added tests that prove my fix is effective or that my feature works.
• [x ] New and existing unit tests pass locally with my changes.

[codecov]

Codecov Report

Attention: Patch coverage is 60.86957% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.19%. Comparing base (87c06cb) to head (032eddc).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
Src/libCZI/CziMetadataBuilder.cpp	0.00%	6 Missing ⚠️
Src/libCZI/XmlNodeWrapper.h	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   66.18%   66.19%   +0.01%     
==========================================
  Files          85       85              
  Lines       10705    10721      +16     
==========================================
+ Hits         7085     7097      +12     
- Misses       3620     3624       +4     

Flag	Coverage Δ
windows-latest	66.19% <60.86%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ptahmose]

At first glance - I'd be surprised if there is an exception/error with the code example you gave. My initial expectation would be:

• The "actual CZI" has one channel by "matter of fact"
• Now, when calling into write_metadata, we are providing "names for channels" and "display-settings" for channels.
• I'd then take the following standpoint: we are providing a map with key channel-no and value "information i.e. name or display-settings", but the semantics should be: only values corresponding to existing channels are used, if there are additional entries in the map (i.e. not corresponding to a channel-no actually existing in the CZI), then they are simply ignored.

In other words - the entries for "channel 1" should be ignored.

[soyers]

The behavior you are describing would also be very sensible. However, I am quite sure that we are getting an error like this. I just checked with libCZI directly (to make sure that I didn't encounter an issue on the pylibczirw side). The tests in libczi can easily be "updated" to provoke the error I described above:

The image shows that I just commented the part of the test setup procedure that writes a subblock for the second channel. In that case the test fails with the "invalid path" error (the one I altered in this PR).

@ptahmose do you think that this is the actual bug? I am having difficulties understading what the indended behavior actually is since both, ignoring the channel and failing, would be reasonable outcomes IMO.

[soyers]

I just did some debugging and I think I was able to locate the root cause of the problem.
In CziMetadataBuilder.cpp the WriteDisplaySettings is implemented like this:

/*static*/void libCZI::MetadataUtils::WriteDisplaySettings(libCZI::ICziMetadataBuilder* builder, const libCZI::IDisplaySettings* display_settings, const std::map<int, PixelType>* channel_pixel_type)
{
    // we determine the highest channel-number that we find in the display-settings object
    int max_channel_index_in_display_settings = 0;
    display_settings->EnumChannels(
        [&](int channel_index)->bool
        {
            if (channel_index > max_channel_index_in_display_settings)
            {
                max_channel_index_in_display_settings = channel_index;
            }

            return true;
        });

    MetadataUtils::WriteDisplaySettings(builder, display_settings, 1 + max_channel_index_in_display_settings, channel_pixel_type);
}

Please note that the max_channel_index_in_display_settings varaible determines exactly what it name implies: the maximum channel index. However, subsequently, this number is interpreted as channel_count.

Therefore, we are facing issues if the channel count is not the same as the maximum channel index, e.g. if the display_settings map (which is provided by the user of the library and therefore not controlled by us) features channel indices that are bigger than the actual channel count of the image (assuming the metadata at Metadata/Information/Image/Dimensions/Channels/ to be the source of thruth). My understanding is that these elements should be ignored instead of causing an error.

I think that there are multiple ways to approach this. One of them would be to change the WriteDisplaySettings function to accept a set of available channels instead of a channel count.
Another option (which to me feels more straight forward) would be to turn things around as we do when creating the channelID and name such that writing the DisplaySettings works as described in the following:

1. Get an iterator id the channels (as they are defined in Metadata/Information/Image/Dimensions/Channels/)
2. Get a channel from the iterator and retrieve its index
3. Check the display settings dict if the retrieved index is present and if that's the case, retrieve the channel ID and name
4. Write the channel entry in the display settings only if channel ID and name could be retrieved (i.e. if the index is present in both, Metadata/Information/Image/Dimensions/Channels/ and the display_settings map.

I would opt for going for this solution, but would be very open for simpler alternative solutions.

[ptahmose]

I first addressed the issue of IXmlNodeRead::GetChildNodeReadonly (-> #108 , #109 ). The exception we got from this method (in the scenario here) was in any case a bug. I will now look into the topic here.

[ptahmose]

I'd now have the following proposal:

• First of all, please see this PR fix behavior of IXmlNodeRead::GetChildNodeReadonly #109 - fixing the exception we had, i.e. when adding the display-settings and there is no information in ../Dimensions/Channel, it is ignored
• This PR also adds a new method ICziWriter::GetStatistics which allows to retrieve the number of channels (that have been written into the CZI)
• Then - please see this branch https://github.com/ptahmose/zeiss-pylibczirw/tree/jbl/proposal_displaysettings_write , or this change ZEISS/pylibczirw@main...ptahmose:zeiss-pylibczirw:jbl/proposal_displaysettings_write - here the idea is that we only add display-settings for the channels that actually exist in the CZI

From the libCZI-API perspective - I'd want to stick with the philosophy "we add all display-settings we are given to the metadata", but now it is straight-forward to ensure on caller-site that we only pass in display-settings for existing channels.

(Note that the code in jbl/proposal_displaysettings_write requires an updated libCZI-version (including #109 ).

[soyers]

Thanks for the proposal! In combination with the previous fix, I like this approach a lot! I'm closing this PR in favor of the implementation you proposed on the caller side :)