Skip to content

Conversation

@hb6688
Copy link
Contributor

@hb6688 hb6688 commented Sep 8, 2025

Removed:
num_rng_blks.

Added:
num_samples_rng_blk. Default = 256. num_samples_rng_blk > 2 x cpi_len. In addition, 5 x cpi_len is recommended

I am also advocating default number of emitters to be reduced from 16 to 12 as CPI Length = 32 pulses. This is to avoid unnecessary RFI mitigation artifact

Please note that num_rng_blks is still kept as a parameter for FDNF.

@hb6688 hb6688 changed the title Replacement of Number of Range Blocks with Number of Range S ST-EVD Replacement of Number of Range Blocks with Number of Range Samples Sep 8, 2025
Copy link
Member

@gmgunter gmgunter left a comment

Choose a reason for hiding this comment

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

Thanks, @hb6688. Here are a few initial comments.

When num_rng_blks=1, all range samples are used to compute EVD
num_samples_rng_blk: int
Number of range samples per range block for computation of sample covariance matrix.
default=256, in general num_samples_rng_blk > 2 x cpi_len is needed
Copy link
Member

Choose a reason for hiding this comment

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

in general num_samples_rng_blk > 2 x cpi_len is needed

Is it needed or recommended?

In the YAML parameter description, you say

It is recommended that this parameter is at least 2 x cpi_len to avoid sample covariance estimation loss and 5 x cpi_len to have a desirable performance.

What do you think about copying that description here in this docstring? It seems more clear and detailed than what you've written here.

Copy link
Contributor Author

@hb6688 hb6688 Sep 8, 2025

Choose a reason for hiding this comment

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

@gmgunter , I found out about the failure in focus.py due to configuration updates in the YAML file and am working on resolving those errors. In addition, I agree with your suggestion that FDNF should also incorporate num_samples_rng_blk instead of keep using num_rng_blks. I wanted to make the changes for ST-EVD first but I think it is a good idea to do it for both right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmgunter , In general, an acceptable sample covariance estimation results in SINR degradation (after RFI processing) of less than ~3dB compared with that of using a true covariance matrix. This requires at least cpi x 2 + 1 number of samples. With the number of samples increasing to cpi x 5, we would have an SINR loss of ~1 dB. The users can set whatever values they desire, but in my code, I raise an error if num_samples_rng_blk < 2 x cpi + 1. With that in mind, I state that 2 x cpi number of samples are needed, but I think I need to make a correction that 2 x cpi + 1 number of samples are needed in order to run ST-EVD.

Copy link
Contributor Author

@hb6688 hb6688 Sep 8, 2025

Choose a reason for hiding this comment

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

I updated the wording for number of range samples per range block as follows. Note that FDNF does not estimate sample covariance matrix, hence having a data block in range with less than cpi x 2 + 1 number of samples is totally fine.

Number of range samples per range block when data blocking
is applied in range direction. It is recommended that this
parameter is at least 5 x cpi_len to avoid degradation in SINR.
In addition, in order to avoid a run-time error for ST-EVD,
this parameter needs to be at least 2 x cpi + 1
num_samples_rng_blk: 256

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that there is no 2 x cpi + 1 restriction for FDNF since there is no sample covariance estimation.

Copy link
Member

Choose a reason for hiding this comment

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

In general, an acceptable sample covariance estimation results in SINR degradation (after RFI processing) of less than ~3dB compared with that of using a true covariance matrix. This requires at least cpi x 2 + 1 number of samples. With the number of samples increasing to cpi x 5, we would have an SINR loss of ~1 dB.

I'm kinda confused about exactly what this means.

By "RFI processing", do you mean RFI mitigation? Is it better to have a ~3dB "SINR degradation" or ~1db? If the RFI power is at least 3dB greater than the signal power, and the RFI processing removes only interference (it doesn't remove any signal), then presumably a 3dB SINR loss is better. But your comment seems to imply that 1dB is preferred.

How did you derive these specific SINR loss numbers?

Number of range samples per range block when data blocking is applied in range direction.

Is there any circumstance when data blocking is not applied in the range direction? If not, I would probably omit the part about "when data blocking is applied in range direction".

in order to avoid a run-time error for ST-EVD, this parameter needs to be at least 2 x cpi + 1

Could you please point out where that error will be raised in the code? I wasn't able to find it.

Copy link
Contributor Author

@hb6688 hb6688 Sep 8, 2025

Choose a reason for hiding this comment

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

@gmgunter

  1. The error checking for minimum number of samples is in compute_evd_cpi.py
  2. With regards to your question about minimum number of samples. I will try to clarify it. In general, you would like to use as many samples as possible to estimate sample covariance matrix R^ where as the true covariance matrix R can be modeled analytically. In the context of interference suppression/nulling, sample matrix inversion (SMI) is often used. As you can tell from the terminology, matrix inversion is applied and this requires a good estimation in R^. With this in mind, a rule of thumb from Reed-Mallete-Brennan (RMB) rule is that at least 2 x N number of samples need to be used, where N represents number of observations or channels. Why 2 x N, because using 2N number of samples gives us a SINR after suppression of within ~3dB of that as if we were to use the true covariance R (SINR_R^ / SINR_R). If you increase the number of samples in estimating R^, then the degradation is less. 5N number of samples gives a SINR loss versus that of using true covariance R of about ~1dB.

We are not doing SMI, neither are we doing MVDR beamformer. (I should point out EVD methodology is a variant of MVDR beamforming). However, we still would like to have a good estimate of the true covariance matrix so we can derive the mutually orthogonal eigenvectors accurately. Therefore we still need to follow the RMB rule for minimum number of samples needed. It is a trade-off between an accurate sample covariance matrix and unnecessary nulling of range samples.

@hb6688
Copy link
Contributor Author

hb6688 commented Sep 9, 2025

I am not sure why the following unit test failed, any ideas?

The following tests FAILED:
47 - test.cxx.isce3.geometry.metadata_cubes.metadata_cubes (Subprocess aborted)
Errors while running CTest

@hb6688
Copy link
Contributor Author

hb6688 commented Sep 9, 2025

Changes in this PR:
ST-EVD and FDNF both uses num_samples_rng_blk intead of num_rng_blks.

Copy link
Contributor

@bhawkins bhawkins left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@hfattahi hfattahi added this to the R05.00.1 milestone Oct 28, 2025
@hb6688
Copy link
Contributor Author

hb6688 commented Oct 29, 2025

@bhawkins @hfattahi , I added a configuration parameter 'use_entire_pulse' to the configuration file because removing num_rng_blks option and replacing it with num_range_samples_blk takes away our ability to use all range samples from a range line since number of range samples is not an a priori.

Copy link
Contributor

@hfattahi hfattahi left a comment

Choose a reason for hiding this comment

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

LGTM.

freq_notch_filter: include('freq_notch_filter_options', required=False)
# Number of range samples per range block when data blocking
# is applied in range direction. It is recommended that this
# parameter is at least 5 x cpi_len to avoid SINR degradation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean SNR?

Suggested change
# parameter is at least 5 x cpi_len to avoid SINR degradation.
# parameter is at least 5 x cpi_len to avoid SNR degradation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or I assume you mean Signal To Interference pulse noise ratio? If yes, I would say spell it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hfattahi I meant Signal to Interference Noise Ratio, but I think SNR degradation is a more accurate description now I think about it.

Copy link
Contributor

@Tyler-g-hudson Tyler-g-hudson left a comment

Choose a reason for hiding this comment

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

Largely looks good, just some documentation changes.
The only thing that needs to be fixed prior to merge is the addition of output_individual_pngs in the QA runconfig parameters; this needs to be removed unless @nemo794 has approved its presence.

num_samples_rng_blk: int, default=256
Number of range samples per range block when data blockin is applied
in range direction. It is recommended that this parameter is at least
5 x cpi_len to avoid SINR degradation. In addition, in order to avoid
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
5 x cpi_len to avoid SINR degradation. In addition, in order to avoid
5 x cpi_len to avoid SNR degradation. In addition, in order to avoid

Following from your discussion with Heresh below

When num_rng_blks=1, all range samples are used to compute EVD
num_samples_rng_blk: int, default=256
Number of range samples per range block when data blockin is applied in range direction.
It is recommended that this parameter is at least 5 x cpi_len to avoid SINR degradation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It is recommended that this parameter is at least 5 x cpi_len to avoid SINR degradation.
It is recommended that this parameter is at least 5 x cpi_len to avoid SNR degradation.

Comment on lines 605 to 615
# True to output one grayscale PNG+KML pair per raster image layer; these
# will be generated in addition to the standard browse image PNG+KML.
# The filename of each additional file will include a suffix noting
# that image's frequency and polarization.
# False to have the primary browse image be the only PNG+KML generated.
# Note: If True, and if the input granule contains only one image,
# then the additional individual PNG will be a duplicate image to the
# primary browse image PNG, but with more descriptive filename.
# Default: False
output_individual_pngs: false

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# True to output one grayscale PNG+KML pair per raster image layer; these
# will be generated in addition to the standard browse image PNG+KML.
# The filename of each additional file will include a suffix noting
# that image's frequency and polarization.
# False to have the primary browse image be the only PNG+KML generated.
# Note: If True, and if the input granule contains only one image,
# then the additional individual PNG will be a duplicate image to the
# primary browse image PNG, but with more descriptive filename.
# Default: False
output_individual_pngs: false

Was this added by mistake? It appears to be unrelated to the PR. Addition of new QA runconfig parameters should be handled by QA version update PR's, per @nemo794

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird... this parameter is already be merged in ISCE3 via #151 . I'm not sure why it is showing up in the diff here.

@hb6688 , perhaps rebasing on the main develop branch will remove this diff? (And the corresponding diff in the schema yaml?) Please address other comments first, and then perform the rebase. Once the rebase is complete, let's confirm that this diff has been removed before merging this PR, otherwise we might have unwanted merge artifacts.

Good catch, @Tyler-g-hudson !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nemo794 @Tyler-g-hudson , I grabbed Brian's PR136 and made changes on top of it to be able to run RFI on dithered PRF scenes. I did not pay attention back then. In short, I had to quickly make this work for PRF dithered cases, and that was probably where it got wrong.

num_range_blocks: 1
# Number of range samples per range block when data blocking
# is applied in range direction. It is recommended that this
# parameter is at least 5 x cpi_len to avoid SINR degradation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# parameter is at least 5 x cpi_len to avoid SINR degradation.
# parameter is at least 5 x cpi_len to avoid SNR degradation.

num_range_blocks: 1
# Number of range samples per range block when data blocking
# is applied in range direction. It is recommended that this
# parameter is at least 5 x cpi_len to avoid SINR degradation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# parameter is at least 5 x cpi_len to avoid SINR degradation.
# parameter is at least 5 x cpi_len to avoid SNR degradation.

Comment on lines 95 to 97
use_entire_pulse: bool
Use all samples in the slow-time pulses for detection if this is True
default = False
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be helpful on a quick read to know if this overwrites what was passed for num_samples_rng_blk or if there will be an error if both values are passed. The answer appears to be the former - that should be clarified here.

Suggested change
use_entire_pulse: bool
Use all samples in the slow-time pulses for detection if this is True
default = False
use_entire_pulse: bool
Ignore any value passed for `num_samples_rng_blk` and instead use all
samples in the slow-time pulses for detection if this is True.
default = False

Copy link
Contributor

@Tyler-g-hudson Tyler-g-hudson left a comment

Choose a reason for hiding this comment

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

That was my only reservation about merging this, the rest LGTM!

@Tyler-g-hudson
Copy link
Contributor

Merging this since Bo is out

@Tyler-g-hudson Tyler-g-hudson merged commit 99e6bf3 into isce-framework:develop Oct 30, 2025
6 of 7 checks passed
Tyler-g-hudson pushed a commit that referenced this pull request Oct 31, 2025
…ples (#136)

* Replaced num_rng_blks with num_samples_rng_blk mechanism for data blocking in range direction.

* Updated ST-EVD function call in focus.py

* Updated schema

* Made correction in focus.py ST-EVD configuration initialization as well as changes in test RSLC YAML files

* Made num_samples_rng_blk a common parameter for both ST-EVD and FDNF.

* Need to add use_entire_pulse to ensure all range samples can be used for detection.

* Removed wavelet caltone removal configuration items from YAML files

* Updated docstring and added error checking.

* docstring updates and removed output_individual_pngs from focus.yaml.

* Removed output_individual_pngs from schema.

---------

Co-authored-by: Bo Huang <bohuang@nisar-adt-dev-5.jpl.nasa.gov>
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.

6 participants