-
Notifications
You must be signed in to change notification settings - Fork 9
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
Create a new InSAR Stripmap workflow #43
Conversation
Hi @mfangaritav, thanks for the PR! I opened mfangaritav#1 with some minor fixes and general cleanup. If those changes look alright to you, feel free to approve and merge that PR. I will provide further feedback and suggestions as comments on this PR. |
Hi @mfangaritav before providing further feedback, it would be helpful for me to test the workflow on my local machine. Do you have a sample command that you've been running locally, that you could share with me? (Or you can just share the reference scene and secondary scene that you've been using for testing.) |
Hi @jtherrmann, thanks for looking at the PR. When I tried it locally I ran the command 'python -m hyp3_isce2 ++process insar_stripmap --username username --password password --reference-scene ALPSRP162831200 --secondary-scene ALPSRP156121200'. The username and password correspond to the asf account |
Fix minor errors and improve formatting
Thanks @mfangaritav! I'll run the command locally to get a better idea of what the workflow does. |
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.
Hi @mfangaritav, sorry for the delayed response. I have some suggestions for how to simplify the insar_stripmap
function. If you approve these changes, then I will likely have some follow-up suggestions for how to simplify the function further. I had one clarifying question regarding a reference to results[0]
(see my feedback below) and I wanted to get your response before making further suggestions.
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Thanks for the suggestions. I committed the changes. Let me know if you have more suggestions or questions |
Hi @mfangaritav I opened another PR from my fork into yours: mfangaritav#2 This PR simplifies the |
Hi @mfangaritav, I left a few more small suggestions. Otherwise, this PR looks good to me! I'm going to hand it over to @forrestfwilliams for final review and approval. |
Also, we should add a Changelog entry for the new workflow. |
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Co-authored-by: Jake Herrmann <[email protected]>
Thank you so much for your input @jtherrmann. |
|
||
This is a placeholder function. It will be replaced with your actual scientific workflow. | ||
def insar_stripmap(user: str, password: str, reference_scene: str, secondary_scene: str) -> Path: | ||
"""Create a Stripmap interferogram |
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.
Various parts of this workflow assume that you are working with ALOS-1 data, even though stripmapApp
can work with other datasets. I don't think this is a problem, but we should put something in the docstring here telling users that this is the case.
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.
At the moment, the only products from asf that I've been able to process with stripmap are the ALOS images. The products from ERS, Radarsat from asf are not compatible with ISCE for now.
@mfangaritav this is looking pretty good to me. Could you add a test for |
@mfangaritav Also a test for |
Sure @forrestfwilliams I'll work on the tests |
Hey @forrestfwilliams, sorry for the delay I was in a conference but I finally made the test for the stripmapApp process here. It super similar the tests for the topsApp process. I commented the last two lines because I do not know what monkeypatch refers to. Let me know if you have comments. |
Thanks @mfangaritav! Hopefully someone from our team will be able to help finalize this PR in the near future. Thanks for your patience! |
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.
@mfangaritav sorry it took me so long to take a look at this, and @jtherrmann thank you for the reminder. The tests you added look good and I've left a suggested change to remove the two lines at the end of the second test that you commented out. Once we get this branch up-to-date with develop
, I'm happy to approve.
Co-authored-by: Forrest Williams <[email protected]>
Thanks @forrestfwilliams!! I removed the lines |
Hey @mfangaritav, could you update the
Then we'll be ready to merge! |
Mario has addressed his concerns
Hi @forrestfwilliams, no problem, I've changed the CHANGELOG.md |
Merged! Thank you for being patient with us on this! |
This PR is to include the stripmap workflow