Drop support for Python 3.7

[dschwoerer]

No description provided.

[ZedThree]

Please could you also bump python_requires in setup.cfg?

[ZedThree]

(Probably also classifiers in the same file)

[codecov-commenter]

Codecov Report

Merging #252 (0a76412) into master (a23da81) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #252   +/-   ##
=======================================
  Coverage   74.11%   74.11%           
=======================================
  Files          13       13           
  Lines        2503     2503           
  Branches      597      602    +5     
=======================================
  Hits         1855     1855           
+ Misses        428      426    -2     
- Partials      220      222    +2     

Impacted Files	Coverage Δ
xbout/plotting/animate.py	46.08% <0.00%> (ø)
xbout/plotting/plotfuncs.py	48.27% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dschwoerer]

It seems the set of required tests needs to be updated - github's repo settings maybe?

[johnomotani]

It seems the set of required tests needs to be updated - github's repo settings maybe?

Yes. Done now!

We should probably also remove python-3.7 in the pytest-min-versions CI job since we're not supporting it any more.

[dschwoerer]

Any idea why the tests time out now?

It cannot be related to the change?

[ZedThree]

No, looking back through previous Action runs, something made them jump up to 6h!

[ZedThree]

This is the last successful run, across all branches, and it took under an hour: https://github.com/boutproject/xBOUT/actions/runs/3135003875
That's commit 1e13242

[dschwoerer]

both xarray and dask where updated to 2022.10.0

[dschwoerer]

I have started runs to debug:

old dask: https://github.com/boutproject/xBOUT/actions/runs/3271985035

old xarray: https://github.com/boutproject/xBOUT/actions/runs/3271980130

[ZedThree]

Yeah, I just checked locally, upgrading to those versions is a massive slowdown

[dschwoerer]

Reported as pydata/xarray#7181

Did you figure out what is slower? Or did you just run the tests?

[ZedThree]

I wondered if it got stuck on the last few tests perhaps and ran pytest -k plot -vv --long. They completed in about 6 minutes. Upgrading xarray and dask from 2022.06/07 (respectively) to 2022.10 and the same tests took a lot longer -- 18% done in 30 minutes.

I've not checked any others yet, but I'll run with --profile now and see if that gives any insight

[johnomotani]

It seems to be xarray rather than dask - I ran pytest with dask==2022.10.0 and xarray==2022.6.0 and tests took about the same time as with my original versions (even a few seconds quicker).

[johnomotani]

Problem seems to be in interpolate_parallel() - if I run time pytest -k "not test_interpolate_parallel" -v, the tests all pass in a reasonable time.

[ZedThree]

See pydata/xarray#7181 (comment) for a bit more detail -- it seems it's basically all coming from copying arrays. Are we copying too much and/or in the wrong places?

[ZedThree]

[johnomotani]

Problem seems to be in interpolate_parallel() - if I run time pytest -k "not test_interpolate_parallel" -v, the tests all pass in a reasonable time.

Sorry, ignore that - I just used the wrong conda environment. All the stuff is slow 😞

[ZedThree]

From what I can tell, the proximate cause is region:1713:_from_region: it does a lot of copying, and copying suddenly got a lot more expensive in 2022.10.

It looks like this is also a substantial part of the runtime even with xarray 2022.06, so it's probably worth trying to optimise it anyway.

[johnomotani]

The regions stuff I was never super-happy with from a performance point of view. Possible optimisation:

• It might be possible to join together the concats here into 2, concatenating guard cells from both sides at once onto the main grid (or maybe even just one, but I'm not sure xarray supports a not-really structured concat), that might reduce the number of copies.
  

  xBOUT/xbout/region.py

  Lines 1738 to 1745 in a23da81

  	# get inner x-guard cells for result from the global array
  	result = _concat_inner_guards(result, ds_or_da, mxg)
  	# get outer x-guard cells for result from the global array
  	result = _concat_outer_guards(result, ds_or_da, mxg)
  	# get lower y-guard cells from the global array
  	result = _concat_lower_guards(result, ds_or_da, mxg, myg)
  	# get upper y-guard cells from the global array
  	result = _concat_upper_guards(result, ds_or_da, mxg, myg)

• The nuclear option would be to completely restructure xBOUT so everything is loaded/handled in separate regions, and doesn't have to do any splitting/recombining. I'm not 100% sure that's a good idea if someone offered to do it for free, but am pretty sure it's not worth the time it would take!
• I'm not sure if it would work, but it might be possible to reduce the number of regions the data is split up into. There are never branch cuts in the x-direction, so maybe for example the whole divertor leg could be one region, rather than the current 2 or 3. This would make getting the guard cells more complicated at branch cuts though...

[dschwoerer]

Just switch to FCI - avoid the separate regions all together :-)

Can we merge this in the mean time, so I can include this in the other PRs, to make sure they don't break the tests?