Fix Chirp Correlator and Cal Type in Raw, NET, RSLC EAP and NEB by rad-eng-59 · Pull Request #231 · isce-framework/isce3

rad-eng-59 · 2026-03-03T00:21:24Z

This PR is the merged combination of the two exiting PR #192 and PR #211 plus some minor improvements.
The main reason for the merge is that these two older PRs are fixing mainly the the same underlying issue with telemetry but for two different applications. Besides, this avoids merging conflict in future while placing the respective common helper functions under appropriate namespaces at once. Thus, once this PR is reviewed and ready to merge, those two older PRs can be safely removed.

Here is the summary of key updates:

Improve Raw parser with helper functions to parse chirp correlator and cal type properly by adding a support for cross-pol products as well as full polarimetric products for Quad-pol case that is currently wrong or missing.
Add a new module to compute residual RX channel imbalances, detect anomalies in received echoes (aka sample slip) where they are used as part of EAP in RSLC.
Update antenna pattern module to use frequency band and caltone for proper synthesis of RX DBF pattern. This requires caltone frequency parser incorporated in Raw L0B parser.
Update noise estimator module (NET) to handle QP properly with correct chirp correlator and cal type values. Also fix some potential biases in noise estimation due to dynamic CPI size in default MEE approach.
Update python API for noise estimator module used in RSLC to estimate NEB. The update adds support for QP w/ correct chirp correlator and cal type telemetry info and force CPI to stay as close as to the input value (or its maximum) for more robust noise power estimation across swath.

…-pub

rad-eng-59 · 2026-03-03T00:23:58Z

@Tyler-g-hudson , FYI your valuable comments from #192 are already incorporated in this PR.

bhawkins

Wow! You've done an admirable job sorting out all the vagaries of the loopback cal data. Honestly, I'm not sure I understand every detail and might've made some stupid comments as a result.

I ended up writing down a lot of comments while reading through the changes, but I think there's only one that I consider a blocker. It's the one to do with the crosspol indexing, and I'll go add some emoji to make it stand out. It may just be a misunderstanding on my part.

python/packages/nisar/products/readers/Raw/Raw.py

bhawkins · 2026-03-04T23:52:58Z

python/packages/nisar/products/readers/Raw/Raw.py

+                                f'Missing dataset {p_ds_i} in {self.filename}.'
+                                f' Detailed error -> {err}'
+                            )
+                            return None


I think I prefer an exception instead of a special return value, but maybe I'll reconsider after seeing the usage. I guess it's a private method (leading underscore in name), so maybe that should be considered as well.

The intention was to handle those old and/or simulated NISAR products and test cases. This behavior is also documented. I'll add more for clarification of the intention.

I understand the intent. My point is that requiring the user to check return values is error-prone and not Pythonic. A mandatory user-side if/else block on the return value is worse than a try/catch block since neglecting the former could cause all sorts of weird behavior, while neglecting the latter halts the program immediately. I'd agree that it might be nice to raise a NotImplementedError here with a nice message rather than the h5py KeyError. Anyways, I don't consider this a blocking issue.

My hands are tight here because of other external factors such as exiting ISCE3 test sample files w/ old formats, broken L0B spec, etc. I had to compromise to avoid a bunch of failures. Also, None is a valid return value not an exception as described in the document. I'm a big fan of try and exception but that won't here. Try your approach and see how things will fall apart.

Let's consider two designs from the user perspective.

Design A:

cc = _parse_chirpcorrelator_from_hrt_qfsp(txrx_pol) if cc is None: # handle L0Bs with missing telemetry

Design B:

try: cc = _parse_chirpcorrelator_from_hrt_qfsp(txrx_pol) except NotImplementedError: # handle L0Bs with missing telemetry

How does Design A handle "test sample files w/ old formats, broken L0B spec, etc." better than B? If you're saying that our unit tests or end-to-end tests won't pass with Design B, then it sounds like we have a lot of places that are missing the necessary logic to avoid incorrect behavior. I actually doubt that's the case since this is a new function, and all the usages should be contained in this PR.

No point of arguing here. We need an offline meeting for this.

bhawkins · 2026-03-05T00:54:48Z

python/packages/nisar/products/readers/Raw/Raw.py

+            except KeyError as err:
+                warn(f'Missing dataset "{p_type}" in '
+                    f'"{self.filename}". Detailed error -> {err}')
+                return None


C style again

The explicit None is for clarity and readability rather than using implicit None return for such scenario where we have different return type. This doesn't make it non-pythonic given this is exceptional scenario where None is returned under the special circumstance, in this case, for missing filed value in L0B. So, I think its existence is more help than harm.

Isn't the Pythonic way to handle an "exceptional scenario" an... Exception? 😆

Nope, because None is a valid return to handle special circumstances as explained in the docstring. Perhaps, there are numpy/scipy functions that return two different types of output depending on the input.

bhawkins · 2026-03-05T00:55:07Z

python/packages/nisar/products/readers/Raw/Raw.py

+                ds_rgl_idx = f5[rgl_idx_path]
+            except KeyError as err:
+                warn(f'Can not parse range line index from HRT. Error -> {err}')
+                return None


C style again

ditto (see previous rational)

python/packages/nisar/products/readers/Raw/Raw.py

python/packages/nisar/noise/noise_estimation_from_raw.py

bhawkins · 2026-03-05T06:33:44Z

python/packages/nisar/antenna/beamformer.py

+                    raise ValueError('"channel_adj_factors" are zeros for all '
+                                     'active RX channels!')
+                rx_wgt *= cor_fact[:, None]
+            elif len(channel_adj_factors) == 1:  # fixed scalar


Not sure I understand the use case for a scalar channel_adj_factors.

A fixed overall scalar for all channels rather than repeating every single value in an array.

Yeah, I just mean we already have two other ways to get a constant scaling into the processor, which is arguably too many already.

I believe there is another reason related to a isce3 test suite and sample product that breaks w/o this conditional block.

python/packages/nisar/antenna/pattern.py

python/packages/nisar/workflows/focus.py

bhawkins

Thanks for the quick revisions! The thing I was worried about turned out to be a red herring. For the sake of completeness, I documented a few minor gripes, but they shouldn't be considered blockers. LGTM.

…or clarity

hfattahi · 2026-03-09T22:48:43Z

We reviewed this PR and agreed that in few places where a function returns None, perhaps the better approach is to raise an exception.

rad-eng-59 · 2026-03-09T23:07:49Z

We reviewed this PR and agreed that in few places where a function returns None, perhaps the better approach is to raise an exception.

Well, there were two flavors discussed in the meeting. I have gone with the latter one which requires minimum update w/o breaking any code while preserve consistency among functions with handling of missing datasets in L0B products. I have tested it over a short QP and DP. Let me know if there is still objection so I can provide the correct updates to Alice in regards to updating L0B process. Note that I'm trying to keep two separate repos, this one and external one, consistent and synced.

bhawkins · 2026-03-09T23:37:44Z

python/packages/nisar/products/readers/Raw/Raw.py

+        True if the L0B product is quad pol otherwise False.
+
+    """
+    return polarization_type_from_raw(raw) == PolarizationTypeId.quad


Suggested change

return polarization_type_from_raw(raw) == PolarizationTypeId.quad

poltype = polarization_type_from_raw(raw)

if poltype is None:

raise ValueError("L0B does not contain polarization telemetry "

"so cannot determine whether it is quad pol")

return poltype

When there's a special return value, not checking for it is almost always a bug. Raising an exception here is another way to address the edge case I identified in chirpcorrelator_caltype_from_raw.

This doesn't make sense here. This function return a boolean not a pol type to decide whether it is quad pol or not. If the respective field is missing in L0B, then the return pol type value will be None and this logic leads to False, that is it won't be quad pol. The warn message already issued due to nonexistent key.

I understand the logic. IMO it's incorrect behavior for is_raw_quad_pol to return False when the telemetry is missing. It could still be QP (for example, there could be four images in the file). "I don't know" isn't the same as "I know it's false."

Next you're going to say, okay fine, let's have this function return None to indicate that. And then I'll say okay great, now we have to check for None in chirpcorrelator_caltype_from_raw and we'll finally have correct C-style error handling in Python. 😜

I have just replaced return with raise and guess what, isce3 tests like the noise estimator (NET) , focus.py, fail, thanks to simulated and/or old L0B products. If the warning message for missing field along with the dcostring of the function are not clear to the user then I am not sure how propagating exception over several module will be more useful for the user whom runs functions blindfoldly. Any suggestion?

Great! I think it's good when the code issues an error when there's a mismatch between the logic and the input data. I'll build this PR with the suggestion above, run the tests, and see if I can come up with a good suggestion for any failures.

I just did some changes to accommodate your suggestions except for this function with a reasonable exception.. Let me test it and upload the commit and see if you like it or not. Give me a few minutes plz.

LOL I stepped away for dinner and you'd already changed everything by the time I got back. You're quick!

…per funcs

rad-eng-59 · 2026-03-10T01:08:23Z

@bhawkins, @hfattahi plz check out the new commit and let me know what you think.

bhawkins

LGTM thanks!

hfattahi

LGTM. Thanks @rad-eng-59 for this important addition. I have tested InSAR workflow with RSLCs generated with this PR and confirm the significant improvement in phase and offsets between pairs acquired with different sampling characteristics.

rad-eng-59 · 2026-03-10T15:06:40Z

@bhawkins , @hfattahi , sorry for the last minute update. I have simply added three lines to better support old or sim L0B products including the edge case for determining quad pol when its respective field/id is missing in L0B . Please check out the new commit and let me know if you both are okay with that prior to merging. Thanks

bhawkins

The latest tweak looks fine to me.

* Add a new mod to nisar.antenna to compute RX imbalances from Raw/L0B * Modify pattern.py to compute and apply RX imbalances per freq band * Update beamformer to allow single scalar for RX channel adjustment * Pass frequency band to EAP block in focus.py * Add a function to compute pulsewidth delay in sequential TX * Modify pattern.py to adjustment RD for the second band in RX DBF * Fix the log in pattern.py * Correct the sign for RD correction of band B in RX DBF pattern * Use slant range diff between A and B in place of pulsewidth delay * Parse caltone frequency from DRT in L0B * Add onboard DBF delay offset of 2.1474 us to RD in pattern.py * Warn if array size in rx channel imbalance class is not 12 * Augment Raw w/ QP-support chirp correlator and caltype functions * Fix noise estimator module to support full QP noise products * Fix TxTRM of TX pattern formaton in pattern.py in case of QP * Fix full NESZ estimation in focus.py for QP * Add support for QP in computing RX channel imbalance * Get caltone freq from runconfig rather than DRT in focus.py for now * Fix handling of CPI update in MEE noise estimator * Handle special case when caltone and chirp freq is the same * Add optional arg `delay_ofs_dbf` to ant pattern ctor in focus.py * Issue warning for too small value of caltone frequency in drt

rad-eng-59 added 28 commits November 30, 2025 16:34

Add a new mod to nisar.antenna to compute RX imbalances from Raw/L0B

3b88d2f

Modify pattern.py to compute and apply RX imbalances per freq band

1759142

Update beamformer to allow single scalar for RX channel adjustment

d0ce1ab

Pass frequency band to EAP block in focus.py

37df7fb

Add a function to compute pulsewidth delay in sequential TX

71dc922

Modify pattern.py to adjustment RD for the second band in RX DBF

ed83d0e

Fix the log in pattern.py

07fae4c

Correct the sign for RD correction of band B in RX DBF pattern

0e2df26

Use slant range diff between A and B in place of pulsewidth delay

0dd72f1

Parse caltone frequency from DRT in L0B

9d3dd59

Set default caltone frequency to none in pattern.py

f28dba7

Add onboard DBF delay offset of 2.1474 us to RD in pattern.py

bce7e7a

Use camel case naming convention for rx channel imbalance class

53a24d9

Fix docstrings, comments, typos, and some minor improvement

2938fc0

Warn if array size in rx channel imbalance class is not 12

18d739f

Merge remote-tracking branch 'upstream-pub/develop' into rximb-dbfpat…

3bf3ae6

…-pub

Fix sign for delay_ofs_dbf in pattern.py

81a83c0

Augment Raw w/ QP-support chirp correlator and caltype functions

1080837

Fix noise estimator module to support full QP noise products

0b3b97c

Fix TxTRM of TX pattern formaton in pattern.py in case of QP

de15d58

Fix full NESZ estimation in focus.py for QP

30c70a9

Add support for QP in computing RX channel imbalance

27bb15e

Get caltone freq from runconfig rather than DRT in focus.py for now

547999d

Fix a bug in rx imbalance module for quasi case

27e9c2e

Merge branch 'caltype-chirpcorr-fix-pub' into rximb-nesz-net-pub

8afc08d

Move l0b-related parsers from rx channel imbalance to raw

5b2a991

Fix handling of CPI update in MEE noise estimator

2ef1466

Improve docstring for module "rx_channel_imbalance_helpers"

76da655

rad-eng-59 requested review from bhawkins and hfattahi March 3, 2026 00:21

rad-eng-59 requested a review from hb6688 March 3, 2026 00:21

rad-eng-59 added the bug Something isn't working label Mar 3, 2026

hfattahi added this to the R05.01.2 milestone Mar 3, 2026

bhawkins reviewed Mar 5, 2026

View reviewed changes

rad-eng-59 added 10 commits March 7, 2026 18:57

Improve docstring and fix typos

2c72c40

Handle special case when caltone and chirp freq is the same

b8df451

More concise logic for the second band detection in ssp

700a7f7

Modify python warn into internal logger info

c704f5b

Add optional arg delay_ofs_dbf to ant pattern ctor in focus.py

4b33a0e

Issue warning for too small value of caltone frequency in drt

6dcd5f9

Minor adjustment to the function return for polarization id

615ffca

Rename "opposite_pol" to "opposite_linear_pol" for clarity

b17db3c

Change a python warn to internal log info

3a8da88

Fix a potential bug in "first_tx_pol_for_quad"

75407ee

bhawkins approved these changes Mar 9, 2026

View reviewed changes

rad-eng-59 added 2 commits March 9, 2026 14:42

Return None for missing polarization type id in L0B

6097e92

Rename "polarization_type_from_drt" to "polarization_type_from_raw" f…

7225b6f

…or clarity

bhawkins reviewed Mar 9, 2026

View reviewed changes

Raise exception instead of returning none for missing keys in raw hel…

bc72a93

…per funcs

bhawkins approved these changes Mar 10, 2026

View reviewed changes

hfattahi approved these changes Mar 10, 2026

View reviewed changes

A new logic for determinig quad pol when poltype is missing in l0b

c8af043

bhawkins approved these changes Mar 10, 2026

View reviewed changes

rad-eng-59 merged commit 1ee718f into isce-framework:develop Mar 10, 2026
7 of 8 checks passed

-    return polarization_type_from_raw(raw) == PolarizationTypeId.quad
+    poltype = polarization_type_from_raw(raw)
+    if poltype is None:
+        raise ValueError("L0B does not contain polarization telemetry "
+            "so cannot determine whether it is quad pol")
+    return poltype

Conversation

rad-eng-59 commented Mar 3, 2026

Uh oh!

rad-eng-59 commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

hfattahi commented Mar 9, 2026

Uh oh!

rad-eng-59 commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rad-eng-59 Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rad-eng-59 commented Mar 10, 2026

Uh oh!

bhawkins left a comment

Choose a reason for hiding this comment

Uh oh!

hfattahi left a comment

Choose a reason for hiding this comment

Uh oh!

rad-eng-59 commented Mar 10, 2026

rad-eng-59 commented Mar 3, 2026 •

edited

Loading

rad-eng-59 commented Mar 9, 2026 •

edited

Loading

rad-eng-59 Mar 10, 2026 •

edited

Loading