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

Small contour ordering bug in Cross-contour_transport.ipynb #383

Open
claireyung opened this issue Jun 12, 2024 · 7 comments
Open

Small contour ordering bug in Cross-contour_transport.ipynb #383

claireyung opened this issue Jun 12, 2024 · 7 comments

Comments

@claireyung
Copy link
Collaborator

The cross-contour transport currently has a bug (even when run with the right kernel -note this is an issue too #327) where the ordering of the contour transports at corners can be slightly wrong. My understanding is that the correct transports are taken across the contour, but when put in the array they can be a slightly different order to the order expected by following the contour (monotonically increasing contour_index). More details on the ordering issue are here: #291 (comment)

This is currently a warning ("Alert") in the notebook. It shouldn't affect cumulative sums of transport or transport calculations with smoothing or averaging, but it might make a difference if you're looking very locally at the transport values.

It would require some changes to the algorithm that determines where cells are positioned relative to each other. I might be missing something but this seems quite tricky as there are there are a lot of possible configurations of the contour when diagonal connections are allowed.

@navidcy
Copy link
Collaborator

navidcy commented Jun 12, 2024

Thanks for opening this!

If we don't know how to resolve this for now, let's at least add a link to this issue in the notebook's intro so that people who use it don't miss it.

@adele-morrison
Copy link
Collaborator

Options for resolving (apart from a straight fix) could be:

  1. Go back to using contours with non-diagonal connections.
  2. Investigate whether we can replace this notebook with xwmb functions if that’s better.

@navidcy
Copy link
Collaborator

navidcy commented Jun 12, 2024

What's xwmb? Could you elaborate please or point to their docs?

@navidcy
Copy link
Collaborator

navidcy commented Jun 12, 2024

(I tried to google and got weird results for Vietnamese lottery tickets...)

@adele-morrison
Copy link
Collaborator

https://forum.access-hive.org.au/t/new-python-package-for-water-mass-transformation-calculation/

Henri will be giving a talk at the COSIMA meeting in 2 weeks about it.

@navidcy
Copy link
Collaborator

navidcy commented Jun 12, 2024

OK gotcha! xwmb! Seems sweet!

Some minor issues with using that package at the moment: I noticed that the package is currently not installable via conda and therefore can't be added in the conda analysis environments we use for analysing COSIMA outputs on NCI. Also the package does not have any tests and this makes it amenable to silent breaking changes as package development continues.

We can help the xwmb team to create some meaningful unit tests and also bit more elaborate tests for the package!

@hdrake
Copy link

hdrake commented Jun 17, 2024

xwmb is overkill for this. I think you just need the more concise sectionate (the relevant dependency of xwmb), which I do have unit tests for but is not yet available for install via conda. My other package regionate essentially allows you to perform the discrete divergence theorem to convert between surface flux integrals (e.g. cross-contour transport) and flux divergences over a volume (e.g. volume-weighted sum of budget terms).

I'll try to register the package on conda ASAP (thanks for the nudge @navidcy) but it might take me a while to get it right.

Any and all contributions from the COSIMA community are welcome!

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

No branches or pull requests

4 participants