-
Notifications
You must be signed in to change notification settings - Fork 257
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
new alosStack, PRF change of ALOS-1, update of TOPS ionospheric correction, new ionospheric filter #175
Conversation
CunrenLiang
commented
Aug 4, 2020
•
edited
Loading
edited
- new alosStack application
- handling PRF change for ALOS-1 raw data
- adding options for discarding burst properties in TOPS ionospheric phase estimation
- updated ionospheric phase filtering for ALOS-2
- a few other minor changes including fixed a bug in ionospheric computation in topsApp.py, fixed problems in getting range sampling frequency, range bandwidth, chirp slope and pulse duration for TerraSAR-X, fixed bug in computing cost flow in snaphu, and others.
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.
Thank you @CunrenLiang and sorry for very late review. It Looks good to me. There is one function that I don't see any where used. It will be great to remove it if it's not needed.
return f2d/np.sum(f2d) | ||
|
||
|
||
def adaptive_gaussian_v0(ionos, wgt, size_max, size_min): |
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.
Is this function used anywhere?
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.
No worries about the late review.
This is an older version of the filter. I am not sure if I will be using it in the future, so I didn't remove it from the file.
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.
ok no worries
@yunjunz and @piyushrpt would you please take a look at this PR. A lot of great contributions from @CunrenLiang are all squeezed in this PR. Of course it would have been nice to see smaller PRs. Maybe for future PRs. |
1. updated filtering of ionospheric phase 2. updated mosaicking of subswath subband interferograms.
fixed bugs in getting range bandwidth, chirp slope, pulse duration and range sampling frequency
It would be great if this pull request can get merged early. I did the rebase. Still one check failed. I think it might be related to docker? I am new to docker. Here is the output
|
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.
The new features and fixes look great, but I want to echo @hfattahi's comment about smaller PRs - it's essentially unfeasible for us to review +13k line diffs. We're able to review and merge more granular PRs with limited scope much more quickly.
minor update
remove author info
delete functions not used
@rtburns-jpl @piyushrpt , I think this docker deploy failure is related to docker credentials? If so, I am just leaving it here until the credentials are updated. |
Could you try merging main into your branch to see if that fixes the deploy issue? |
use if false instead of commenting out codes
remove update info from code.
for resolving conflicts with recent commit
Still failed at password step @rtburns-jpl . |
ping @piyushrpt - didn't we run into something similar a while ago? I don't recall what we did to solve it :/
|
add comments about identifying bad pairs
correct variable name
I think the problem might be with "docker login -u $DOCKER_USER -p $DOCKER_PASS". These two envs need to set somewhere. For failsafe, it's better to use -n check before calling it. Anyway, I advised @CunrenLiang to rename his branch from main; that might be the reason to force a deploy check on his PR.
|
Hey, sorry about all the CI issues. I think they're all resolved now, so if you merge main, you should get all green checkmarks. |
Thank you for the update @rtburns-jpl . Looks like the same problem in deploy is still there. The error message is the same. |
Ugh, yep. Well, at least the build job is working now. Anyway, I'll try to look this over to the best of my ability next week so we can merge this soon. |
I think Lijun's suggestions might be promising. I tried to change my branch name from 'main' to something else, but it looks like it's not easy to do it without deleting my current branch and re-fork. So perhaps we can try to change the circleci configure file. |
Ah yeah, maybe the deploy job is just checking if the branch name is 'main' without also checking that it's not running on a fork. Honestly, don't worry about it. The build/test jobs were what I was concerned about and those are all fine now. |
Yes, Lijun just explained it with more details. It should be OK to just merge it. Then I can rename my fork to other names instead of 'main', so that it won't deploy again for my future PRs. |
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.
Between this and PyCuAmpcor, we have a good number of new features in isce2, so I'll plan to cut a v2.5 release after merging.
It may be better to include this pull request in v2.5, as many people are waiting to use the new features in this pull request. Right now they have to use the code in my repository.
|