Closed EJFielding closed 2 years ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Note that I left the notebook file name the same as before to make it easier to compare to the previous version in ReviewNB.
View / edit / reply to this conversation on ReviewNB
dbekaert commented on 2022-05-23T04:20:50Z ----------------------------------------------------------------
Update this cell to only report in the comment which site is to be used for review.
EJFielding commented on 2022-05-23T18:25:49Z ----------------------------------------------------------------
updated
View / edit / reply to this conversation on ReviewNB
dbekaert commented on 2022-05-23T04:20:50Z ----------------------------------------------------------------
Line #8. import pickle
Why is pickle needed?
now removed
View / edit / reply to this conversation on ReviewNB
dbekaert commented on 2022-05-23T04:20:51Z ----------------------------------------------------------------
Not sure why pickle is needed.
EJFielding commented on 2022-05-23T18:23:12Z ----------------------------------------------------------------
This was a leftover from Kang's notebook. No longer needed, so I deleted it.
This was a leftover from Kang's notebook. No longer needed, so I deleted it.
View entire conversation on ReviewNB
Could we drop NISAR_SES_
from the notebook filename as well?
@yunjunz David asked me to keep the name the same so that the new version can be easily compared to the old version in ReviewNB.
Maybe someone can send another renaming PR after this is merged?
Can use the git mv
command to rename it to a new name. I think that would still allows for comparision
If you recall our video meeting last time, together with Marco, you could also use right click --> rename
within jupyter to do this, similar to git mv
. Both will allow the comparison.
I renamed both the Coseismic and Secular notebooks. I used git mv
which works for GitHub, but seems to confuse ReviewNB. Anyway, it is no longer important to compare to the old Secular notebook.
View / edit / reply to this conversation on ReviewNB
yunjunz commented on 2022-05-23T22:18:20Z ----------------------------------------------------------------
Line #5. site='MojaveD173'
Not a big deal. But if we use site_name
here, then we could run site = sites[site_name]
once and use site
instead of sites[site]
for the rest of the notebook.
The rest of the PR looks great.
View / edit / reply to this conversation on ReviewNB
yunjunz commented on 2022-05-23T23:32:12Z ----------------------------------------------------------------
We should ues Tides
instead of Tide
as we correct for all the tidal components. You might want to ensure this throughout the notebook.
The last commit by @nabolfat was some heading and table of contents updates for the Coseismic notebook.
Added code to calculate Method 2, added site MojaveD173, and changed to use that for Secular validation