search

xgcm / aerobulk-python

A python wrapper for aerobulk (https://github.com/brodeau/aerobulk)
GNU General Public License v3.0
14 stars 4 forks source link

v0.2.5 skin is broken #38

Open jbusecke opened 2 years ago

jbusecke commented 2 years ago

Sorry for the oversight. The fact that tests show as passed in the CI while they fail in the fortran code is really dicey. We (I especially since that has happened multiple times to me at this point) have to check each of the CI actions and confirm that pytest actually completed the test and did not crash.

Back to the problem at hand though. Unfortunately in https://github.com/xgcm/aerobulk-python/pull/34 I forgot to reactivate the test for the skin xarray wrapper, which lead to an actually passing CI! I noticed the missing test parametrization in #35, but missed that the tests did not pass.

So it seems that threadsafe somehow broke the skin but not the noskin method but only in the case of certain chunking schemes. Very weird, ill investigate now.

jbusecke commented 2 years ago

I have dug as deep as I understand in #39, but do not see a quick solution for this.

Should we actually remove the threadsafe from the skin wrapper for now? This would enable the code to work, but probably not be very useable. But it might be more desirable than code that just crashes?

@paigem @rabernat

jbusecke commented 2 years ago

We have decided to shelf this issue for now. In #40 the threadsafe was removed from the skin logic. It now works but is still slow on a threaded scheduler. We will focus on driving the research forwards and reevaluate afterwards if we actually require the skin functionality.