-
Notifications
You must be signed in to change notification settings - Fork 4
Set the dimension for R_closed_wall and Z_closed_wall to 'closed_wall' #191
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
Conversation
9c56444
to
0f65a48
Compare
The 'boututils' module was merged into the 'boutdata' package, so we need to depend on 'boutdata' instead of 'boututils' now. Also need the latest version for a bugfix.
xBOUT detects grid files by checking that they do not have a "t" dimension. However, boututils.Datafile defaults to writing a 1d array with a "t" dimension, so the "R_closed_wall" and "Z_closed_wall" variables were previously written with a "t" dimension, which made xBOUT try to load the grid files as if they were dump files (which fails). Fix this by specifying the name of the dimension as "closed_wall".
Python-3.8 is no longer available on Github Actions runners, so remove. Add more recent versions of Python to the tests, and for the examples, etc. that run on a single version, just use the default (presumably the latest version).
There seems to be a bug in xarray's imports, which is causing some CI jobs to fail.
Something (presumably changed rounding errors) was making some conditionals evaluate to the wrong result for a separatrix segment around the core. For these segments, the first and last points of the FineContour and PsiContour are in the same place, so the FineContour should not need to be extended, but without the tolerance introduced in this commit, one was being extended, causing errors in poloidal_distance_xlow. Also increase tolerances in 'with rounding errors' test, as Python-3.13 has changed things slightly (maybe different random-number generation?).
Hope to catch bugs caused by updates in dependencies, etc.
5319950
to
ae08894
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johnomotani, just one minor correct (that I think is necessary) and one question. Cheers, Tim
.github/workflows/pythonpackage.yml
Outdated
@@ -59,7 +62,7 @@ jobs: | |||
pip install .[tests] | |||
- name: Integrated tests | |||
run: | | |||
pip install pytest xarray | |||
pip install pytest "xarray!=2025.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth putting these version constraints in the pyproject.toml
? Will they impact users of hypnotoad or just the CI tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xarray's only in pyproject.toml
as an optional dependency for tests. Also, they've already released a bugfix release xarray-2025.6.1 that fixes the issue, so nobody should need to use 2025.6.0 (it was only the current version for a couple of days!), and we could probably just drop these version restrictions now.
Probably needed to avoid problems due to YAML interpreting special characters. Co-authored-by: Timothy <[email protected]>
xarray has made a bugfix release, so we can just use the latest version again.
ecf8110
to
0f1759d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
push: | ||
pull_request: | ||
schedule: | ||
- cron: '25 1 3 * *' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend something like '25 1 * * 3'
- as that is disabled after 3 months, so would run around only 2 times. If you run it weekly, you will get a run closer to when the cron CI is disabled
xBOUT detects grid files by checking that they do not have a "t" dimension. However, boututils.Datafile defaults to writing a 1d array with a "t" dimension, so the "R_closed_wall" and "Z_closed_wall" variables were previously written with a "t" dimension, which made xBOUT try to load the grid files as if they were dump files (which fails). Fix this by specifying the name of the dimension as "closed_wall".
Requires boutproject/boutdata#124. Tests won't pass until that's released.
doc/whats-new.md
with a summary of the changes