asdf.commands.diff will report that contents differ for any arrays including NaNs

[schlafly]

I think we want asdf.commands.diff to not report differences when comparing two identical arrays which both contain NaNs in the same positions. This comparison
https://github.com/asdf-format/asdf/blob/main/asdf/commands/diff.py#L268-L269
needs to have the equal_nan=True argument for that to occur. Otherwise arrays including NaNs will always report differences here.

Only vaguely related to this issue, but I was also hoping for more verbose output from asdf.commands.diff. In the particular file I'm looking at:

>>> asdf.commands.diff([
    'test_ramp_fitting_step0/truth/r0000101001001001001_01101_0001_WFI01_rampfit.asdf',
    'test_ramp_fitting_step0/r0000101001001001001_01101_0001_WFI01_rampfit.asdf'],
    minimal=False, ignore=['roman.cal_logs', 'asdf_library', 'history'])
        ndarrays differ by contents

I get only "ndarrays differ by contents." I would have appreciated being told something more like 'tree['roman']['data'] ndarrays differ by contents' so that I knew more than just some array somewhere in the file differed.

[braingram]

Thanks for bringing this up. I just had a brief discussion with @nden about this and other asdf diff changes. I'm going to try converting this to a "discussion" (in part to see how that works) but also to solicit input from @perrygreenfield and @eslavich on what verbose output might look like.

It sounds like the changes so far are (and I might update this portion of this comment as the discussion continues):

• allow option to specify equal_nan=True for array comparisons
• create a more verbose output for array differences

[schlafly]

Digging into this more... I'll be more verbose tomorrow, but using asdf.commands.diff(...) has more issues for Roman:

• we use units. When these files are opened they get _force_raw_type=True, which means that little dictionaries like {'unit': 'DN', 'value': {'source': 0, 'datatype': 'float32', 'byteorder': 'little', 'shape': [6, 4096, 4096]}} propagate down for comparison instead of the actual binary data. The 'source' bits there I think are indeterminate and trigger errors that that the binary files differ, even when the binary data is identical.
• we probably need some tolerances; I expect that we can get differences at machine precision and might like to ignore that.

[braingram]

Thanks for taking a look and the follow up. I opened up a related romancal PR (it appears that the comparisons against truth files using compare_asdf will always pass): spacetelescope/romancal#867

It seems like there are many thing to address. I do not have much experience with asdf.commands.diff (from your comments I believe you have more than me at this point). It might be safe to assume that some major rework will be needed to address all of the needs for roman (more investigation might reveal how much work). Perhaps we break it down into:

• What is minimally required now to get the regression tests to a valid True/False if the files differ? Is the failed test output written to artifactory like with jwst? If so, the files could be inspected to see the details about the difference. This might be addressable by selecting the right set of paths in the asdf tree to ignore (in addition to the source issue for units that you mentioned above).
• When the comparison fails, what output is useful? Is there a general formula for this or will this need to be configured for each use (roman vs other users of asdf.commands.diff)
• What are existing bugs in asdf.commands.diff? My brief testing today has lead to some errors but it might be from improper usage.

[schlafly]

Thanks. Yes, your PR is where I started this particular issue. I don't want to merge that now until we've fixed more of the issues it reveals.

Yes, the highest priority is to get True / False if the files differ "meaningfully." I've been running the regression tests locally where I can do the inspection you describe. So far, I've found three cases:

1. some bug on the romancal side where a WCS is present in the 'truth' file but not in the 'input' or 'output' files for the corresponding step, for a step that has nothing to do with WCS. I don't understand that yet but it will need to be addressed on the romancal side.
2. Files with identical NaNs comparing False even when identical; the first part of this issue.
3. Files with units not comparing the binary data and instead comparing the dictionaries with source in them, and giving spurious results.
   There may be more; as you noted, there are a number of failures. But those are the ones I've stumbled on so far. There is also
4. I suspect we want to use a tolerance rather than demanding exact bitwise identity. In my experience I get machine precision level differences just from running on one machine vs. another. But I actually haven't seen that yet so maybe I'm making that up?

The most useful bit to report is to say which fields have differences. Currently I just get "contents differ" for each ndarray it looks at, without naming the ndarray. Then I know where to look to see what has changed. There are other things like min / max absolute and fractional difference and the number of differing elements that would also have some value, at much lower priority.

You've probably already done something more sensible, but FWIW, this is what I put together to find the first set of not NaN cases:

    try:
        different = not array_equal(array0, array1, equal_nan=True)
    except TypeError:
        # This branch ends up being called, e.g., for arrays of strings.
        different = not array_equal(array0, array1, equal_nan=False)
    if different:
        differences.append("contents")

[braingram]

To summarize the conversation/work so far. Romancal is considering transitioning away from using asdf.commands.diff to using deepdiff. See: spacetelescope/romancal#868
asdf may want to consider the pros/cons of transitioning asdf.commands.diff to use deepdiff vs updating the command to fix the issues exposed during this conversation.

[braingram]

Furthermore, based on offline discussions we should consider deprecating (in version 3.1) asdf.commands.diff which would allow us to remove jmespath as a dependency and document how a user might compare 2 asdf trees using deepdiff.

[ddavis-stsci]

Just as an additional data point I use the command asdftool diff file1 file2 fairly regularly. Mostly when updating regression tests and for simple tests of when INS updates L1 and L2 files to check and see if it is what I am expecting. At this point it is mostly checking keywords. It would be better to have more info on the data arrays than "ndarrays differ by contents" if only something like,
"ndarray NAME differ by contents"