Fix bugs in Cross-contour_transport take 2

[adele-morrison]

Closes #327 and closes #403.

I accidentally closed the other PR on this. This PR includes all of @navidcy's and @anton-seaice's suggestions from the old PR. It still needs a review from someone who can go through and check the code / results are sensible.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2024-07-11T13:59:29Z
----------------------------------------------------------------

space after commas... sorry for being OCD

Also, why do we rename yu_ocean to y_ocean? Can't we rename it to latitude which is English and understandable by anybody, not only by MOM5 experts?

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2024-07-11T13:59:30Z
----------------------------------------------------------------

Wouldn't it be better if we made the unit conversion before plotting and fixing the unit attributes of the data array?

adele-morrison commented on 2024-07-23T23:19:44Z
----------------------------------------------------------------

Do you mean skip this plot? We could do that, but I feel like it's useful to show the user what we've got as of this point in the code, so they understand that code after this is just tidying up the x-axis labels.

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

hrsdawson commented on 2024-07-17T01:04:12Z
----------------------------------------------------------------

I think this should read "Values south of the contour have been filled with 0 and thus are a different colour", or alternatively "Values north of the contour have been filled with -1000 and thus are a different colour"?

And below: "This is done by looping through the contour points and determining in which directions there are zeros (above contour) and -100 (below contour)." -> "This is done by looping through the contour points and determining in which directions there are zeros (below contour) and -1000 (above contour)"

[review-notebook-app]

View / edit / reply to this conversation on ReviewNB

hrsdawson commented on 2024-07-17T01:04:12Z
----------------------------------------------------------------

Perhaps this code could be shortened/replaced by:

target_lons = [-280., -240., -180., -120., -60., 0, 60., 80.]

distance_indices = np.zeros(len(target_lons))

   

for i in np.arange(0, len(lon_along_contour.values)):

  for j, lon in enumerate(target_lons):

    distance_indices[j] = np.argmin(np.abs((lon_along_contour.values - lon)))

This returns the following distance indices: [0. 602. 1625. 2597. 3474. 4830. 5780. 6051] compared to the original/existing code which returns: [0. 604. 1626. 2599. 3475. 4831. 5781. 6051.].

[hrsdawson]

I've added some comments and run the code several times. Looks sensible to me. It doesn't work in conda environments older than 23.07 (fails at cell 13 with path_vertices = (sc.get_paths()[0]).vertices) but I'm not sure this is a problem, since people probably won't use older environments?

[navidcy]

And below: "This is done by looping through the contour points and determining in which directions there are zeros (above contour) and -100 (below contour)." -> "This is done by looping through the contour points and determining in which directions there are zeros (below_ contour) and -1000 (above contour)_"

I'd even suggest we avoid using the syntax:

"In the case A (B) one goes up (down) and that feels really nice (bad)"

which is essentially an if-else statement. I find it cumbersome to read -- my brain needs to go back and forth. A much more friendlier version is:

"In the case A one goes up and that feels really nice; contrary when B things go down and that's bad."

[navidcy]

I've added some comments and run the code several times. Looks sensible to me. It doesn't work in conda environments older than 23.07 (fails at cell 13 with path_vertices = (sc.get_paths()[0]).vertices) but I'm not sure this is a problem, since people probably won't use older environments?

Nah, that shouldn't be a problem. That environment is more than a year old anyway... soon it will be deleted. It's good to move on :)

[navidcy]

This returns the following distance indices: [0. 602. 1625. 2597. 3474. 4830. 5780. 6051] compared to the original/existing code which returns: [0. 604. 1626. 2599. 3475. 4831. 5781. 6051.].

Why in your arrays above some numbers appear as integers (eg 6051) and some as floats (eg 6051.)? If the extra dot is not needed perhaps remove it? If it's needed perhaps let's add the trailing zero to read more like English, (eg 6051.0)?

[hrsdawson]

This returns the following distance indices: [0. 602. 1625. 2597. 3474. 4830. 5780. 6051] compared to the original/existing code which returns: [0. 604. 1626. 2599. 3475. 4831. 5781. 6051.].

Why in your arrays above some numbers appear as integers (eg 6051) and some as floats (eg 6051.)? If the extra dot is not needed perhaps remove it? If it's needed perhaps let's add the trailing zero to read more like English, (eg 6051.0)?

This was just a copy-paste error sorry. All were floats.

[navidcy]

This was just a copy-paste error sorry. All were floats.

No need to apologise!

[adele-morrison]

Do you mean skip this plot? We could do that, but I feel like it's useful to show the user what we've got as of this point in the code, so they understand that code after this is just tidying up the x-axis labels.

---

View entire conversation on ReviewNB

[adele-morrison]

@hrsdawson and @navidcy, thanks for the review. I've included all of your suggestions above now. Are one of you happy to approve?

[navidcy]

oh I missed this -- let me run now to see if all is good

[navidcy]

looks great!

[navidcy]

oh sorry... I run the wrong thing!

[navidcy]

the actual notebook actually runs and looks great!

[navidcy]

lgtm

[access-hive-bot]

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/newest-conda-env-kernels-silently-change-model-output-analysis-results/2010/10