Coherence Change Detection using Coherence in IfgramStack.h5#37
Coherence Change Detection using Coherence in IfgramStack.h5#37yunjunz merged 11 commits intoinsarlab:mainfrom
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:46Z
mgovorcin commented on 2022-02-24T05:08:14Z Applied all comments, thanks @yunjunz |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:47Z Line #5. import os, sys, numpy as np please use single line import for each module, as below: import os |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:48Z Line #19. mintpyObj.open() This should be |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:48Z "mean" and "stack" in the variable name and labels seems a little bit complicated to me, could we move the averaging into |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:49Z Line #15. ccd_histmatch = exposure.match_histograms(mco_cohStack, mpre_cohStack, multichannel=False) Nice! |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:50Z Could we merge this cell into the one above, to be consistent with the similar cells below? |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-18T02:45:51Z Not a big deal: maybe you want to use a sequential colormap like |
|
Thank you @mgovorcin, this is a very interesting notebook! I left a couple of inline comments above. Could you also add a link to this notebook in the README under "applications"? |
- applied Yunjun commits - make the notebook shorter
|
Applied all comments, thanks @yunjunz View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:12Z Line #32. mask :: float - mask value, used to mask the CCD values below the defined value I would recommend:
|
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:12Z Line #125. sys.exit() It's better to use |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:13Z Line #156. method = '' How about limiting yunjunz commented on 2022-03-01T01:09:35Z I find the
# methods
methods = {
'diff':'Difference',
'hist':'Histogram Matching',
'ratio':'Ratio',
}
|
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:14Z Line #25. co_coherence_data = [] Would suggest to use |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:14Z Could you add a short paragraph on the rationale of the "coherence thresholding"? It would help the reader to understand this part quickly? |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-02-27T00:34:15Z Line #15. for mthd in enumerate(methods): Use |
|
Thank you @mgovorcin for addressing my previous comments. The notebook looks much simpler and cleaner now, which is very nice! I added a few more minor suggestions on the newer version, which I hope you could address without much effort. |
|
I find the
# methods
methods = {
'diff':'Difference',
'hist':'Histogram Matching',
'ratio':'Ratio',
}
View entire conversation on ReviewNB |
change method option definition from: 'diff', 'hist', 'ratio' to 'difference', histogram_matching', 'ratio' make the code for plot function shorter and add option to flip axes if option orbit_direction used
|
@yunjun I addressed all new comments, thank you for suggesting them as they improve the notebook greatly.
|
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-03-03T20:31:06Z We are almost done here @mgovorcin. This ceill used to plot the initial CCD result without refinement, but not shown in this latest version. Shall we still plot it, or was it on purpose to remove it? mgovorcin commented on 2022-03-07T18:06:39Z there should be a plot, as I did not remove plotting of initial results. It seems that ReviewNB failed to generate the plot. I noticed that it started acting weird after the third PR yunjunz commented on 2022-03-07T19:24:50Z Right, the plot is shown in your forked repo: https://nbviewer.org/github/mgovorcin/MintPy-tutorial/blob/main/applications/coherence_change_detection.ipynb. |
|
View / edit / reply to this conversation on ReviewNB yunjunz commented on 2022-03-03T20:31:07Z Not a big deal at all: the shown plots is a little bit distracting, I don't see calling plot functions here. Why did it happens? mgovorcin commented on 2022-03-07T18:11:38Z I believe Jupyter notebook plots this image as an output of save_kmz.py. I added the line "%matplotlib notebook" before this line in the new version to prevent plotting .kmz file. |
|
there should be a plot, as I did not remove plotting of initial results. It seems that ReviewNB failed to generate the plot. I noticed that it started acting weird after the third PR View entire conversation on ReviewNB |
|
I believe Jupyter notebook plots this image as an output of save_kmz.py. I added the line "%matplotlib notebook" before this line in the new version to prevent plotting .kmz file. View entire conversation on ReviewNB |
Little cleanup following PEP8 style and renaming of variables to fit snake_case style
|
Right, the plot is shown in your forked repo: https://nbviewer.org/github/mgovorcin/MintPy-tutorial/blob/main/applications/coherence_change_detection.ipynb. View entire conversation on ReviewNB |
yunjunz
left a comment
There was a problem hiding this comment.
Looks great! Thank you @mgovorcin for this very cool notebook!
No description provided.