Skip to content

Fix misleading attribute truncation and hidden tail differences - #373

Open
sylvesterkaczmarek wants to merge 2 commits into
nasa:developfrom
sylvesterkaczmarek:bugfix/issue-371
Open

sylvesterkaczmarek wants to merge 2 commits into
nasa:developfrom
sylvesterkaczmarek:bugfix/issue-371

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Sep 10, 2026 •

Copy link
Copy Markdown

Fixes #371.

Compare complete attribute values while keeping the existing five-item report preview. Add a separate display-value option to the outputter so shortening a row cannot change its classification, difference marker, or keep-only-differences filtering. Only append an ellipsis when items were omitted, and render nested arrays without NumPy's implicit comparison truncation.

Validation

Initial validation: macOS arm64, Python 3.12.11. Latest review follow-up: macOS arm64, Python 3.11.16.

  • Full non-integration suite: 106 passed, one authenticated integration test deselected.
  • End-to-end regressions cover differences at the sixth and 1500th items, NetCDF root/variable attributes, and HDF5 root/variable attributes, including --only-diffs and --exit-code CLI behavior. The variable-level HDF5 cases failed before the follow-up fix.
  • Empty, short, nested, scalar, and equal long values, output classification, and markers are covered. HDF5 reference-array dereferencing and consistent root/variable fixed-string formatting are explicitly tested.
  • Existing text/CSV/Excel golden checks passed unchanged. The observed ATL06 fixture count remains 4,978, so no fixture or expected-count updates were needed.
  • Ruff checks/formatting and git diff --check passed. Changelog updated.

Live Earthdata and Windows/Linux runs were not performed. The byte-decoding changes in #372 and palette isolation in #369 remain separate.


📚 Documentation preview 📚: https://ncompare--373.org.readthedocs.build/en/373/

@codecov

codecov Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.29%. Comparing base (5931766) to head (4e18ee9).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #373      +/-   ##
===========================================
+ Coverage    97.11%   97.29%   +0.17%     
===========================================
  Files            9        9              
  Lines          589      591       +2     
===========================================
+ Hits           572      575       +3     
+ Misses          17       16       -1     
Flag Coverage Δ
python-3.11 97.29% <100.00%> (+0.17%) ⬆️
python-3.12 97.29% <100.00%> (+0.17%) ⬆️
python-3.13 97.29% <100.00%> (+0.17%) ⬆️
python-3.14 97.26% <100.00%> (+0.18%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Refreshed onto current develop and resolved the overlap with the newer fixed-length HDF5 string handling. The current implementation preserves that upstream decoding behavior while comparing complete attribute values and truncating only the displayed report values.

The branch is 0 behind, mergeable, and GitHub Verified. Validation:

  • focused attribute/getter tests: 31/31 passed
  • full standard non-integration suite: 98/98 passed, 1 integration test deselected
  • Ruff passes
  • git diff --check passes

Could a maintainer review the refreshed head when convenient?

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Refreshed onto current develop. The branch is now 0 behind, mergeable, and GitHub Verified. Focused attribute/getter validation passes 31/31 tests and Ruff is clean. The repository's Receive PR workflow is green. Could you re-review the refreshed head when convenient?

@danielfromearth

Copy link
Copy Markdown
Collaborator

That's great that you already rebased this, thanks!

One thing I'm still hoping to see is the rendering looking the same for root and for variable attributes. Right now, I see [NASA, GSFC, JPL] for the root attribute and ['NASA' 'GSFC' 'JPL'] for the variable one.

And a bigger thing: variable attributes don't seem to be picking up the new full-value comparison, and so a row gets counted as matching (no ***, the _difference_marker). For example, two files whose variable attribute differs at element 1500 finish without indicators of a difference (difference count is 0, not in --only-diffs, --exit-code of 0, etc). Same data as a root attribute gives a *** marker and exit 1. That seems like part of issue #371 too.

Looks like line 584 and line 596 turn those into strings before get_attribute_value_as_str acts on them, and handing it the raw value instead (like we're doing for root attributes) could sort it. I'd be happy to help sort out additional test case(s) if you'd like.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Thanks @danielfromearth — both findings were valid. Updated in 4e18ee9: HDF5 variable attributes now retain full values until comparison and formatting, so tail differences and root/variable string displays behave consistently. Object references remain supported. Added variable-level, element-1500, CLI --only-diffs/--exit-code and reference regressions. Full non-integration suite: 106 passed, one integration test deselected; Ruff, formatting and diff checks pass. The PR description is also refreshed.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Does the variable-attribute fix address the remaining comparison and formatting concerns? Let me know what else needs changing.

@danielfromearth
danielfromearth self-requested a review October 21, 2026 21:05

@danielfromearth danielfromearth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks pretty good. One thing, though: DIMENSION_LIST and REFERENCE_LIST now seems to show up as <HDF5 object reference> instead of names, and on the ATL06 fixtures that affects all 541 rows. The existing reference fixture mentions DIMENSION_LIST but builds a plain (1, 1) ref array, which is probably why nothing caught it. Along with the fix, I think we need a new test to check that swapped dimension scales are reported.

Comment thread CHANGELOG.md

### Fixed

- Compare complete attribute values before shortening report lists, so differences after the fifth element remain detectable. Only append an ellipsis when list items are actually omitted ([#371](https://github.com/nasa/ncompare/issues/371)).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Compare complete attribute values before shortening report lists, so differences after the fifth element remain detectable. Only append an ellipsis when list items are actually omitted ([#371](https://github.com/nasa/ncompare/issues/371)).
- Compare complete attribute values before shortening report lists, so differences after the fifth element remain detectable, and render HDF5 variable attributes the same way as root attributes. Only append an ellipsis when list items are actually omitted ([#371](https://github.com/nasa/ncompare/issues/371)) ([**@sylvesterkaczmarek**](https://github.com/sylvesterkaczmarek))

This branch has not been deployed

No deployments
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