Skip to content

update notebooks to reflect change in opentopo#46

Merged
sssangha merged 8 commits intomasterfrom
main
Aug 20, 2022
Merged

update notebooks to reflect change in opentopo#46
sssangha merged 8 commits intomasterfrom
main

Conversation

@bbuzzanga
Copy link
Collaborator

@bbuzzanga bbuzzanga commented May 5, 2022

No description provided.

@bbuzzanga bbuzzanga requested a review from sssangha May 5, 2022 23:57
@dbekaert
Copy link
Collaborator

dbekaert commented May 12, 2022

@sssangha Can you review?

@bbuzz31 Could you add the capability to do notebook review? Perhaps check with @yunjunz or mintpy repo in case of questions how to enable it.

@yunjunz
Copy link
Collaborator

yunjunz commented May 14, 2022

@bbuzz31 Could you add the capability to do notebook review? Perhaps check with @yunjunz or mintpy repo in case of questions how to enable it.

It should be straightforward to link ReviewNB (https://www.reviewnb.com/) with the Github repo, one might need the admin permission to enable it.

@dbekaert
Copy link
Collaborator

dbekaert commented May 17, 2022

Thanks @yunjunz I added it in now. Hopefully works for the next PR.

dbekaert
dbekaert reviewed May 17, 2022
View reviewed changes
JupyterDocs/ariaDownload/ariaDownload_tutorial.ipynb Outdated
@@ -54,9 +54,11 @@
"name": "stdout",
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022

Choose a reason for hiding this comment

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

  • Output syntax has now some typo's compared to the past.
  • The version string would benefit from more description. Does this only take that version or would it take preference for that version if there are two versions. for example newest does that take only the last version or the best available version etc?

Reply via ReviewNB

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

Good question: how's this updated help string: "Specify version as str, e.g. 2_0_4 or all prods; default: 'newest. All products are downloaded. Unspecified versions are stored in "workdir"/duplicated_products'.

JupyterDocs/ariaDownload/ariaDownload_tutorial.ipynb Outdated
@@ -54,9 +54,11 @@
"name": "stdout",
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The output cell did not work. The filename is different. Fix as needed. Also did this change between versions?


Reply via ReviewNB

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

it changed in ariaTools in April 2020 to ensure that files don't get overwritten (commit: fce76a6f972f98b248826355eedcf9abce7bdab5)

JupyterDocs/ariaDownload/ariaDownload_tutorial.ipynb Outdated
@@ -54,9 +54,11 @@
"name": "stdout",
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Aria-download failure. Can you check if this is working?


Reply via ReviewNB

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

There was a bug (fixed) but also asf_search cannot currently download KMZ/KML. I removed these cells from the notebook.

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

It looks like I may be able to push a little to get ASF to incorporate. Not a priority but is nice to have, especially for students playing around with their own study areas. What do you think?

JupyterDocs/ariaDownload/ariaDownload_tutorial.ipynb Outdated
@@ -54,9 +54,11 @@
"name": "stdout",
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The verbose does not seem to print which products anymore. Is that intentional as it contradicts the statement below?


Reply via ReviewNB

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

Nope that's a bug. Fixed.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:06Z
----------------------------------------------------------------

Can you also do a catch all for using stage data with wget? So if you are offline you could potentially still use stage data. Look at nisar repo how they did this: https://github.com/nisar-solid/ATBD/blob/main/methods/coseismic/Coseismic_Requirement_Validation-Ridgecrest_asc.ipynb

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:07Z
----------------------------------------------------------------

same comment about verbose mode. That needs some fix in the actual code.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:08Z
----------------------------------------------------------------

Would not be badd to add in the notebooks what the duplicated_products folder is all about.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:08Z
----------------------------------------------------------------

  • Same comment on version
  • Would not be bad to add more information to GACOS. Does it support both product variants of GACOS or not? If so add it to the description for it. Could also add in the URL to GACOS website.


bbuzz31 commented on 2022-05-26T17:47:40Z
----------------------------------------------------------------

Per Sim: I believe so, but it prioritizes the new tif formats for sure

So it looks for duplicates and discards binary format if tif exists.

Help is updated to reflect this.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:09Z
----------------------------------------------------------------

  • "need to have"?
  • API key of what? API key for openTopo

dbekaert
dbekaert reviewed May 17, 2022
View reviewed changes
JupyterDocs/ariaMisclosure/ariaMisclosure_tutorial.ipynb
@@ -1,10 +1,5 @@
{
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ITs not clear what API key for what.

  • Explain first also why you need a key for what you need it and then how to get it.

Also you just need it period, so does not add much information.

  • Remove on same computer as running this notebook as cloud is not a computer.

Reply via ReviewNB

JupyterDocs/ariaMisclosure/ariaMisclosure_tutorial.ipynb
@@ -1,10 +1,5 @@
{
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do you need to keep the " " and put key on between or not? clarify


Reply via ReviewNB

Copy link
Collaborator Author

@bbuzzanga bbuzzanga May 24, 2022

Choose a reason for hiding this comment

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

Hmm no you can just do echo KEY > ~/.topoapi

Is this what you meant?

JupyterDocs/ariaMisclosure/ariaMisclosure_tutorial.ipynb
@@ -1,10 +1,5 @@
{
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is no plot in your notebook. Please check if this is an error and needs fix in code.


Reply via ReviewNB

JupyterDocs/ariaMisclosure/ariaMisclosure_tutorial.ipynb
@@ -1,10 +1,5 @@
{
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

same no plot


Reply via ReviewNB

JupyterDocs/ariaMisclosure/ariaMisclosure_tutorial.ipynb
@@ -1,10 +1,5 @@
{
Copy link
Collaborator

@dbekaert dbekaert May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

same. think you did not run cells, but would be good to check if this still works.


Reply via ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:13Z
----------------------------------------------------------------

Line #19.            !aws s3 cp --region us-east-1 --no-sign-request s3://asf-jupyter-data/aria-data/ariaPlot.zip ariaPlot.zip

Would be good to add the wget also, see my earlier comment in other notebook:


@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:13Z
----------------------------------------------------------------

It would be good if users can see some sort of progress.

  • Add message download has started.
  • Can we still have some sort if progress bar?
  • Then say download complete?

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:14Z
----------------------------------------------------------------

see comment in other notebook about duplicated products folder.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:15Z
----------------------------------------------------------------

same comment about the version as earlier notebook

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:16Z
----------------------------------------------------------------

Line #36.            !aws s3 cp --region us-east-1 --no-sign-request s3://asf-jupyter-data/aria-data/ariaTSsetup.zip ariaTSsetup.zip

Same comment of using wget as other notebook. Note saying to fall back to wget if the aws command fails.


@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:17Z
----------------------------------------------------------------

not sure about the traceback messages?


bbuzz31 commented on 2022-05-26T18:18:32Z
----------------------------------------------------------------

I had an older version of asf_search.py installed that errors out if products are already downloaded. New version just outputs a warning.

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:17Z
----------------------------------------------------------------

same comment about duplicated product folder

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:18Z
----------------------------------------------------------------

same statement as other notebook

@review-notebook-app
Copy link

review-notebook-app bot commented May 17, 2022 •
edited
Loading

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-05-17T08:07:19Z
----------------------------------------------------------------

same comment as other notebook

@dbekaert
Copy link
Collaborator

dbekaert commented May 17, 2022

@bbuzz31 the rendering is not yet working, so best is to click on respond via the review-notebook tool. I did my review using the tool outside github as it was not showing here (I believe next time we will have it working).

In summary:

  • I think some cells did not work, so would check if this is an error or not
  • the verbose does not print product download summary
  • the download does not give progress which could be problematic
  • the version of ARIA-tools did we capture that now correctly in the code? @yunjunz had suggest a specific approach similar to mintpy where you pull it from some sort of version file. Suggest we adopt that.
  • some more description would be beneficial for the program help function.

@review-notebook-app
Copy link

review-notebook-app bot commented May 24, 2022

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@bbuzzanga bbuzzanga marked this pull request as draft May 24, 2022 19:21
@bbuzzanga Review Notebook App
Copy link
Collaborator Author

bbuzzanga commented May 26, 2022 •
edited
Loading

Per @sssangha : I believe so, but it prioritizes the new tif formats for sure

So it looks for duplicates and discards binary format if tif exists.

Help is updated to reflect this.

View entire conversation on ReviewNB

@bbuzzanga Review Notebook App
Copy link
Collaborator Author

bbuzzanga commented May 26, 2022

I had an older version of asf_search.py installed that errors out on duplicate products. New version just puts a warning.

View entire conversation on ReviewNB

@bbuzzanga bbuzzanga mentioned this pull request May 26, 2022
Merged
@bbuzzanga
Copy link
Collaborator Author

bbuzzanga commented May 26, 2022

@dbekaert You caught a lot of bugs, thanks for going through these in such detail!

I think I've addressed all the comments, and commits are here and aria-tools/ARIA-tools#306.
The one outstanding task is the progress bar for the download.

@bbuzz31 the rendering is not yet working, so best is to click on respond via the review-notebook tool. I did my review using the tool outside github as it was not showing here (I believe next time we will have it working).

In summary:

* I think some cells did not work, so would check if this is an error or not

* the verbose does not print product download summary

* the download does not give progress which could be problematic

* the version of ARIA-tools did we capture that now correctly in the code? @yunjunz had suggest a specific approach similar to mintpy where you pull it from some sort of version file. Suggest we adopt that.

* some more description would be beneficial for the program help function.

@bbuzzanga bbuzzanga marked this pull request as ready for review May 26, 2022 22:22
@sssangha
Copy link
Collaborator

sssangha commented Aug 3, 2022

@bbuzz31 apologies, but this had slipped me. However, I had reshuffled the .netrc/.topoapi generation step to the top "prep" sections and made it so that a user explicitly has to enter their credentials if these files don't exist. Please refer to PR #47

@sssangha
Copy link
Collaborator

sssangha commented Aug 3, 2022

@bbuzz31 following my initial message, I believe it would be easier to merge my suggested changes here. Namely, I've added the following:

  1. Add topoapi check + cleanup .netrc checks and move all to prep stage(s) at top of notebook.
  2. If a given file doesn't exist, and explicit warning is passed, plus a prompt for users to enter their credentials.

I'll keep the aforementioned PR in draft form for now for reference, but I intend to merge this to master

@bbuzzanga
Copy link
Collaborator Author

bbuzzanga commented Aug 3, 2022

@sssangha there are numerous other small changes in this PR besides the topo check. Can you clarify whether you're going to merge this branch and #47?

@sssangha
Copy link
Collaborator

sssangha commented Aug 3, 2022 •
edited
Loading

@bbuzz31 my plan is to close #47 and merge this one. I had essentially moved the .netrc and .topoapi checks to the top within the Prep sections. We could merge and proceed with this branch

I just kept the other branch open for now so that my commits here may be a bit more transparent

@bbuzzanga
Copy link
Collaborator Author

bbuzzanga commented Aug 3, 2022

Oh I see now. Sounds good!

@sssangha sssangha merged commit ddf704e into master Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbekaert dbekaert dbekaert left review comments

@sssangha sssangha sssangha approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bbuzzanga @dbekaert @yunjunz @sssangha