-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
removing azimuth burst ramps caused by ionosphere in topsStack #600
Conversation
These updates are for removing azimuth burst ramps caused by ionosphere in the ionospheric correction in topsStack. The updates are only within this module.
I would like to ask if there is a timeline in merging this PR. Thank you! |
I have tested this PR recently with a stack and did not encounter issues. Great contribution, thank you @CunrenLiang ! Sorry that it took me so long to run this PR on my datasets. Looks like I need to find a Chilean region with even stronger burst ramp jumps to demonstrate this. But this is what I have now, and it is working. Perhaps we can merge it @hfattahi , @yunjunz unless more tests are needed? The outputs were stored in the following directories.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall. Thank you @CunrenLiang for the PR and @yuankailiu for the testing!
I added a few suggestions below, trying to resolve the conflicts since stackSentinel.py
has been updated (in #505, #702) since this PR. Or maybe you could do a git rebase to resolve yourself @CunrenLiang?
+ resolve the conflicts since stackSentinel.py has been updated (in isce-framework#505, isce-framework#702)
I am testing this PR to check if it would help with scalloping issue in my interferograms (workflow is correlation stack). In stackSentinel the function correlationStack is missing creation of generate_burst_igram run-file, and for merge_burst_igram run-file, its calling burstIgram_mergeBurst function instead of igram_mergeBurst |
Hi @chintals, as per my understanding, the I think this PR is intended for |
Hi @yuankailiu, I apologize if my understanding is incorrect, but as far as I know, interferogram workflow creates interferogram and also performs SNAPHU or icu unwrapping. Correlation workflow only creates wrapped phase (filtered and unfiltered) and associated correlation file. And the ionospheric phase estimation creates lower and upper interferograms regardless of workflow, but for this PR to get more precise azshifts, coregistration should ideally be NESD. |
Hi, sorry I need to take back my words, and you are right, it should create those interferograms run files regardless of workflow. I think what you mentioned has been fixed in #635 and committed. So, version 2.6.3 does not have this issue in correlationStack. As @yunjunz said, once this PR is rebased on the current 2.6.3 version, it should be okay. I did it on my branch, but I don't have access to add my commit here I think. After I do the rebase, it looks like the
|
Hi @yuankailiu Thank you! Yes, this is what I expect and am running at the moment. I manually corrected the file on my end and figured I should let folks know as well. |
Hi @CunrenLiang and @rtburns-jpl, I would like to ask if this PR can be merged with the main branch. |
Any news when this PR will be incorporated in ISCE2 since it also has some changes on ALOS-2 data processing. |
Hi @rtburns-jpl and @hfattahi, can this PR be merged since it contains important tools for estimating and correcting ionosphere in Sentinel-1 IFG stack? It also contains fixes in processing ALOS-2 data. |
Thanks @bjmarfito for your patience. @rtburns-jpl let's make progress with this PR and few other open. |
Co-authored-by: Zhang Yunjun <[email protected]>
Co-authored-by: Zhang Yunjun <[email protected]>
Co-authored-by: Zhang Yunjun <[email protected]>
Co-authored-by: Zhang Yunjun <[email protected]>
Co-authored-by: Zhang Yunjun <[email protected]>
Thanks @yunjunz for helping with revolving the conflicts. They look fine to me. I revolved another conflict. The pull request is ready to merge. I think now it's time to do it in case there are further conflicts in the future. What do you think @hfattahi ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good to me to be merged!
These updates are for removing azimuth burst ramps caused by ionosphere in the ionospheric correction in topsStack. The updates are only within this module.