Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an example for along-isobath averaging #416

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

taimoorsohail
Copy link
Collaborator

@taimoorsohail taimoorsohail commented Jul 8, 2024

Hi, I am posting a new Contributed Example which averages properties along an isobath - useful for Antarctic margins analysis. Let me know if you have thoughts!

closes #397

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@navidcy
Copy link
Collaborator

navidcy commented Jul 8, 2024

Seems that you added two files?

@taimoorsohail
Copy link
Collaborator Author

Yeah sorry - I accidentally included a random change I made to the ContributedExamples/Cross-slope_section_potrho_cc.ipynb file - please delete if possible before pushing the new file :)

@navidcy
Copy link
Collaborator

navidcy commented Jul 9, 2024

@taimoorsohail is there a reason you wanted to make this a "Contributed Example"?

We decided to just have "Examples" and they should all be documented and in good shape; see #407.

@taimoorsohail
Copy link
Collaborator Author

Ah sorry, I didn't know that we had moved on! Yes happy to add it to "Examples" if everyone is happy with the level of documentation and commenting.

@navidcy navidcy changed the title Added an along-isobath averaging code Added an example for along-isobath averaging Jul 9, 2024
@navidcy
Copy link
Collaborator

navidcy commented Jul 9, 2024

if everyone is happy with the level of documentation and commenting.

We'll let the review process determine that. :)

@adele-morrison
Copy link
Collaborator

Hey @taimoorsohail, out of interest, is it necessary to use st_edges_ocean for the bins? Would it be possible in theory to use a more even binning (say spaced every 50m in isobath depth) and then use ht/hu for computing the histogram?

@adele-morrison
Copy link
Collaborator

Currently the time average is done just before plotting, but the time information is not used. Perhaps time averaging and loading earlier could make it run faster?

@adele-morrison adele-morrison self-requested a review July 11, 2024 20:35
@@ -0,0 +1,1700 @@
{
Copy link
Collaborator

@adele-morrison adele-morrison Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can get away with only one set of plots? Since they are identical except for the x-axis labels. Would having different example axis labels on different sub-panels be too confusing?


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is definitely possible. I tend to maximise all possible combinations when writing out an example, just to account for more possible use-cases. But yes, In theory we could just do a 1x3 plot, with each subplot showing the three different x-axes (i.e., ht_bins, normed_area and pseudo-lat) for a single variable (e.g. temperature). I think the different x-axis labels might be confusing but yes, also a possibility.

@adele-morrison
Copy link
Collaborator

Also, if you edit your top post to add:
closes https://github.com/COSIMA/cosima-recipes/issues/397,
it will automatically close the issue when this is merged.

@taimoorsohail
Copy link
Collaborator Author

Hey @taimoorsohail, out of interest, is it necessary to use st_edges_ocean for the bins? Would it be possible in theory to use a more even binning (say spaced every 50m in isobath depth) and then use ht/hu for computing the histogram?

Hey @adele-morrison! In theory, it should be OK. I had some issues masking ht/hu with st_edges_ocean for the bins, as ht and hu cover a larger range of depths than the bottom depth of the variables (see https://forum.access-hive.org.au/t/difference-between-bottom-bathymetry-variable-ht-and-actual-data-in-access-om2-01/2221). This meant I was getting more "jagged" averages which only really affected the "prettiness" of the visualisation. I didn't try reducing the bin spacing though.

Currently the time average is done just before plotting, but the time information is not used. Perhaps time averaging and loading earlier could make it run faster?

Won't time-averaging after binning account for the temporal variability within each bin? Whereas time-averaging before binning would get rid of that? I'm not sure...

@@ -0,0 +1,1700 @@
{
Copy link
Collaborator

@navidcy navidcy Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cell looks like the same as the cell above?

then why not making it a function and just call it twice with different inputs?


Reply via ReviewNB

@adele-morrison
Copy link
Collaborator

Hey @taimoorsohail how is this one going? This would be super useful code for a bunch of people, so would be great to get it included!

@taimoorsohail
Copy link
Collaborator Author

Hi @adele-morrison thanks for the prod! I think I was confused how to handle varying vertical grids in this code. At the moment, I bin using a new bottom depth variable I create based on the last depth where T/S/rho data is available - this aligns with st_edges_ocean. In reality, the ht/hu variable contains the 'actual' bottom depth. It would be more accurate to use this value, but would involve stretching/shrinking the T/S/rho profiles, right? Anyway, your thoughts are welcome...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Build an along-isobath averaging visualisation for scalar and vector properties
3 participants