Fixes bugs in Cross-contour_transport.ipynb

[adele-morrison]

Closes #327 and closes #403. This fixes the bugs, so this notebook now works with the default conda environment. I also changed the contour, so it now uses the 1000m Antarctic isobath, which is more commonly used in the COSIMA community.

I'm not sure what the remaining alert at the top of the script is about:

Alert: After including the additional cases the contour number doesn't always monotonically increase along the contour. At the moment, the two indices that are set at the same time are adjacent numbers, whereas if you were following the contour you'd expect their numbers to be 2 apart with the other coordinate in between.

It looks ok to me?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[navidcy]

Thanks @adele-morrison!

So GitHub is not that clever and doesn't understand English so well you need to edit the first post to rephrase to

Closes #327 and closes #403

Otherwise only #327 is linked with the PR...

PS: me talking to GitHub: "Come on GitHub -- pull it together!"

[navidcy]

Also: perhaps add the label "bug" since this PR fixes #307?

[navidcy]

I pushed some minor formatting tweaks. This looks good! At least the plot at the end looks how it used to be... But I'd like some experts on the issue give a review on the algorithm-science bit so I'm not approving...

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2024-07-05T02:59:15Z
----------------------------------------------------------------

Line #17.        #if mask_loc%100 == 0:
Line #18.        #    print('mask for x/y transport at point '+str(mask_loc))

These two lines seem to be just commented out code.

If we don't need this code let's just delete it???

[navidcy]

Nice job!
Just made a minor comment on some code that is commented out... If we don't want that let's delete it.

I'll wait to hear from people who worked on this regarding the science-correctness of what you've propose here @adele-morrison.

[navidcy]

Note: @adele-morrison, because I pushed few commits you might need to git pull on your fork before you start editing further. If you have uncommitted changes that you don't mind loosing then git checkout will remove those and take you back to your last commit and then you can git pull... ;)

[claireyung]

Hi @adele-morrison, regarding

I'm not sure what the remaining alert at the top of the script is about:

Alert: After including the additional cases the contour number doesn't always monotonically increase along the contour. At the moment, the two indices that are set at the same time are adjacent numbers, whereas if you were following the contour you'd expect their numbers to be 2 apart with the other coordinate in between.

It looks ok to me?

The alert is referring to the issue #383 with figures/output in the comment here: #291 (comment) which happens when you have diagonals in your contour and one offset cell. The problem comes from cell 26 in your notebook, e.g.

    elif (contour_masked_above_halo[index_j, index_i]==0) and (contour_masked_above_halo[index_j, index_i+2]==0):
        mask_x_transport[index_j, index_i-1] = 1
        mask_x_transport[index_j, index_i] = -1        
        mask_x_transport_numbered[index_j, index_i-1] = new_number_count
        mask_x_transport_numbered[index_j, index_i] = new_number_count+1

where the x transports on either side of the offset cell should be 2 apart in terms of the along contour number and there is a y transport in between. I think this is hard to generalise as it depends on the orientations whether this elif section gets turned on.

So I'd recommend to keep the warning until the issue is resolved (or notebook replaced!)

[navidcy]

If the alert is referring to an issue then let's add a reference to the issue in the notebook! If @adele-morrison is confused I imagine that a lot of other people would be confused as well. So, I agree -- keep the warning -- but ensure that is clear to people why the warning is there (e.g. some of the explanation that @claireyung wrote just above I believe belongs in the notebook).

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-09T07:06:50Z
----------------------------------------------------------------

Line #1.    %matplotlib inline

You can delete this line

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-09T07:06:51Z
----------------------------------------------------------------

Line #5.    import netCDF4 as nc

and this isnt used

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-09T07:06:52Z
----------------------------------------------------------------

Line #1.    start_time = '2170-01-01'

These three lines about time don't make sense in this spot in the notebook anymore

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-09T07:06:52Z
----------------------------------------------------------------

You can just do this with :

contour_mask_numbered = np.arange(1,len(x_contour)+1)

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-09T07:06:53Z
----------------------------------------------------------------

Line #1.    contour_mask = 0 * ht

can be :

contour_mask = xr.zeros_like(ht)