Skip to content

Fix the InSAR Product Spec Issues - #345

Open
xhuang-jpl wants to merge 40 commits into
isce-framework:developfrom
xhuang-jpl:fix_spec_20260731
Open

Fix the InSAR Product Spec Issues #345
xhuang-jpl wants to merge 40 commits into
isce-framework:developfrom
xhuang-jpl:fix_spec_20260731

Conversation

@xhuang-jpl

@xhuang-jpl xhuang-jpl commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

This PR is to fix the spec issues for the InSAR, which are related to
#297 , #304 , #312 , #318 , #319

Xiaodong Huang added 30 commits September 19, 2023 20:40
@nemo794

nemo794 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

For a reviewer, it's tedious and error-prone to manually verify that all items have been addressed correctly in a case of Multiple-Issues-to-One-PR. Too high of a likelihood of human error for my comfort.

Could you please close this PR, and reopen the fixes as new PRs, where each PR addresses one Issue re: InSAR?

Notes:

  • Please do not auto-close the Issues which still contain TODOs for RSLC, GSLC, and GCOV
  • For each change you propose in the PRs, ask yourself questions like: "What is our NISAR convention for saving Boolean flags? Will this be saved as a Python string or a fixed-length string? Are all conditional cases addressed by this fix?

Thanks

@xhuang-jpl

Copy link
Copy Markdown
Contributor Author
  • For each change you propose in the PRs, ask yourself questions like: "What is our NISAR convention for saving Boolean flags? Will this be saved as a Python string or a fixed-length string? Are all conditional cases addressed by this fix?

Thanks @nemo794 , it is a good idea to split this to small PRs, but some fix is just few lines code change, it might not be necessary to open so small PRs. However, if from the commit message, we can track the issues that the fix will address. Thanks for the notes too.

@nemo794

nemo794 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

For curiosity's sake, I asked Claude for the better approach. Initially he agreed with the all-in-one PR, then with more info he changed his response to recommend separate PRs:

When working in a large, shared GitHub repo that is generating several outputs science HDF5 products, each with  
  tightly-defined specifications. Suppose there are several minor discrepancies between the HDF5 being generated   
  and the specifications, e.g. typos, dtype discrepancies, etc. These discrepancies are categorized and noted      
  across multiple (5-8) different GitHub Issues. When addressing these Issues, is it preferred to address all      
  Issues in a single PR, or to have one PR to address the discrepancies noted each Issue? Please give your         
  reasoning. Thanks!                                                                                               

  Thought for 9s (ctrl+o to expand)

⏺ For HDF5 spec compliance fixes (typos, dtype discrepancies, etc.), I'd recommend bundling them into a single PR,
  with the issues grouped by logical area if the total is large. Here's the reasoning:

  Why bundle (in your case):

  1. Related changes: All fixes address the same root problem—spec compliance discrepancies in HDF5 output. They're
  variations on a theme, not independent features.
  2. Testing efficiency: HDF5 validation likely runs against complete files. You'd be running the same validation
  suite 5-8 times for separate PRs, and the "does output match spec?" question is holistic—partial fixes may still
  fail validation.
  3. Review efficiency: Reviewers can see the full picture of spec compliance work in one pass. Context switching
  across 8 similar PRs is draining; one coherent PR is easier to understand.
  4. Avoid merge conflicts: Spec fixes likely touch the same schema files, generator code, or validation logic.
  Sequential PRs would create conflicts and rebase churn.
  5. Atomic release: Your release notes can say "Fixed all HDF5 spec discrepancies" instead of listing 8 minor
  items. Downstream users get the corrected output in one update.

  When to split instead:

  - If one fix is high-risk (major dtype change affecting downstream consumers) while others are trivial
  - If issues are truly independent (e.g., one issue is about metadata, another about compression
  settings—different domains)
  - If you need to merge incrementally due to external dependencies
  - If your team culture strongly prefers "one issue = one PR" for tracking

  Practical approach:

  Create one PR that references all 5-8 issues in the description (e.g., "Fixes #123, #124, #125..."). Organize
  commits by issue if helpful for review ("Fix dtype in dataset X (#123)", "Correct typo in attribute Y (#124)").
  This gives traceability while keeping the work atomic.

  Tradeoff: If reviewers push back or CI fails on one fix, you might need to split it out—but for minor spec fixes,
  that's unlikely.

✻ Churned for 20s
    
❯ All issues are independent of each other; there will not be merge conflicts. Also, the reviewer will need to     
  have open all Issues anyways, and each Issue has multiple sub-bugs to track. Does that change your answer?       
  
  Thought for 11s (ctrl+o to expand)

⏺ Yes, that changes my answer significantly. With those details, I'd recommend one PR per Issue. Here's why:
     
  Why separate PRs now makes sense:
     
  1. Progress tracking: Each Issue has multiple sub-bugs to track. One mega-PR means you can't close Issues
  incrementally—either you merge nothing until all 5-8 Issues are done, or you close Issues before their fixes are
  merged (confusing). Separate PRs let you ship Issue #123's fixes, close it, and move on while #124 is still in
  review.
  2. Review focus: If the reviewer already needs all Issues open to check sub-bugs, one PR covering all 8 means
  they're context-switching through 8×N sub-bugs in a single review session. Separate PRs let them focus deeply on
  Issue #123's sub-bugs, approve, then tackle #124 fresh.
  3. Incremental deployment: Since they're independent, you can merge and validate Issue #123's fixes in production
  while #124 is still being reviewed. You don't hold completed work hostage to unrelated work.
  4. Revert granularity: If Issue #123's fix causes a regression, you revert one small PR. With a bundled PR,
  reverting means losing all 8 fixes.
  5. Blame/bisect clarity: git bisect or git blame will point to "Fix dtype discrepancies in temperature dataset
  (#123)" not "Fix all HDF5 spec issues." Easier to trace which fix introduced a change.
  6. PR size: 5-8 Issues × multiple sub-bugs = potentially massive PR. Smaller PRs get faster reviews.

  The key insight:

  "Multiple sub-bugs per Issue" signals these aren't trivial one-liners—each Issue is substantial enough to be a
  coherent unit of work. Since your reviewer is already organized around Issue boundaries (they'll verify each
  sub-bug), aligning PR boundaries with Issue boundaries reduces cognitive overhead.

  One PR per Issue is the way.

@hfattahi hfattahi added this to the R05.03.0 milestone Aug 11, 2026
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.

3 participants