Fixes bugs in Cross-contour_transport.ipynb

adele-morrison commented 4 months ago

Closes https://github.com/COSIMA/cosima-recipes/issues/327 and closes https://github.com/COSIMA/cosima-recipes/issues/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[bot] commented 4 months ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

navidcy commented 4 months ago

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 commented 4 months ago

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

navidcy commented 4 months ago

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[bot] commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/COSIMA/cosima-recipes/issues/383 with figures/output in the comment here: https://github.com/COSIMA/cosima-recipes/issues/291#issuecomment-1747990241 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 commented 4 months ago

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[bot] commented 4 months ago

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[bot] commented 4 months ago

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[bot] commented 4 months ago

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[bot] commented 4 months ago

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[bot] commented 4 months ago

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)

COSIMA / cosima-recipes

Fixes bugs in Cross-contour_transport.ipynb #406