Update on Coseismic using staged data

View / edit / reply to this conversation on ReviewNB

dbekaert commented on 2022-06-03T16:12:28Z ----------------------------------------------------------------

@EJFielding were we not going to use the alert boxes to call more emphasis that this is a final results? Its still a bit hidden away and one needs to carefully read the text.

EJFielding commented on 2022-06-03T18:06:34Z ----------------------------------------------------------------

Yes, I had added

<div class="alert alert-warning">
Final result Method 2—
  68% of points below the requirements line is success
</div>

It somehow got lost in this version.

EJFielding commented on 2022-06-03T18:07:36Z ----------------------------------------------------------------

@nabolfat can you add that back to the notebook here?

dbekaert commented on 2022-06-05T14:24:54Z ----------------------------------------------------------------

This is still to be adressed

EJFielding commented on 2022-07-07T23:27:26Z ----------------------------------------------------------------

@dbekaert the boxes are there in the notebook, but ReviewNB is not showing them.

dbekaert commented 2 years ago

A tiny detail not taking anything away of the notebook. I noticed that it might perhaps be better if we do not label the appendix with a number sequential to the previous section (e.g. rather use A1, B1 etc). I fear a reviewer might just be scrolling down completely to the end and miss the results in like section 6 as being the final?

EJFielding commented 2 years ago

Yes, I had added

<div class="alert alert-warning">
Final result Method 2—
  68% of points below the requirements line is success
</div>

It somehow got lost in this version.

View entire conversation on ReviewNB

yunjunz commented 2 years ago

GitHub flagged conflicts in the "Secular_*" notebook. Could you rebase your branch against the latest upstream/nisar-solid main branch, to resolve the conflict?

EJFielding commented 2 years ago

@nabolfat can you add that back to the notebook here?

View entire conversation on ReviewNB

nabolfat commented 2 years ago

Sure. I rebased my changes. Let me update and submit the notebooks separately.

On Fri, Jun 3, 2022 at 11:20 AM Eric Fielding @.***> wrote:

@.**** commented on this pull request.

In methods/coseismic/Coseismic_Requirement_Validation.ipynb https://github.com/nisar-solid/ATBD/pull/35#discussion_r889221726:

@@ -96,11 +96,11 @@
 "- [6.3. Amplitude vs. Distance of Relative Measurements (pair differences)](#M2ampvsdist2)\n",
@nabolfath Can you change this to:

print('\n site disp-los [cm]')

Reply via ReviewNB https://app.reviewnb.com/nisar-solid/ATBD/pull/35/discussion/

— Reply to this email directly, view it on GitHub https://github.com/nisar-solid/ATBD/pull/35#pullrequestreview-995305293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHRW47ERQY4AXJAMBZ6EJL3VNJEHPANCNFSM5XVWNSJA . You are receiving this because you were mentioned.Message ID: @.***>

nabolfat commented 2 years ago

I rebased my last 3 commits. I have made ready two notebooks including all the changes for Secular and Coseismic. @yunjunz can you please verify that the conflict is resolved and it is ok for me to add new commits?

yunjunz commented 2 years ago

@nabolfat I am gonna re-open this PR so that I could run git rebase on your branch.

yunjunz commented 2 years ago

@nabolfat Github conflict is resolved now.

dbekaert commented 2 years ago

This is still to be adressed

View entire conversation on ReviewNB