Cambridge-ICCS / ONEFlux

Open Network-Enabled Flux processing pipeline
Other
0 stars 0 forks source link

initial modularisation of 'cpdBootstrap' #38

Closed j-emberton closed 3 weeks ago

j-emberton commented 1 month ago

initial modularisation of 'cpdBootstrap'

Currently breaks regression tests - unknown why

Added pytest-xdist to parallelise running of tests (currently set to 1 in pyproject.toml)

j-emberton commented 1 month ago

I've had an initial look at modularising and testing cpdBootstrap.

I think the use of varargin in some functions is breaking the regression tests somehow. The problem is that 'Stats2' can no longer be interrogated properly. However all unit tests pass.

Would appreciate a 2nd pair of eyes on it.

tztsai commented 1 month ago

I've had an initial look at modularising and testing cpdBootstrap.

I think the use of varargin in some functions is breaking the regression tests somehow. The problem is that 'Stats2' can no longer be interrogated properly. However all unit tests pass.

Would appreciate a 2nd pair of eyes on it.

It turns out that the output Stats2 is a double string '"..."', so after json decoding it's still a string.

It's because in setup_Stats.m you added

if size(varargin)>0;
    Stats = jsonencode(Stats);
end

And in the cpdBootstrap....m file there is also another jsonencode if size(varargin) > 0 .

dorchard commented 1 month ago

Hm my tests are failing with a lot of Undefined function 'XXX' for input arguments of type 'int64' (or `double').

j-emberton commented 1 month ago

Still tests failing after your commit @tztsai , it's just different tests now. Did you run the test suite before you committed your change?

image
tztsai commented 1 month ago

The tests were passed on my machine image

j-emberton commented 1 month ago

I run 58 tests, whereas you have only run 52. Have you synced all tests locally?

tztsai commented 1 month ago

It's up to date with origin. But I only ran the unit tests. Now there are 58 tests. image

j-emberton commented 1 month ago

It's up to date with origin. But I only ran the unit tests. Now there are 58 tests. image

Do they all pass?

tztsai commented 1 month ago

Some of them failed because of this error. It's due to the getfield function and seems unrelated to Stats. Others are due to the extra input arguments to setup_Stats in the test, so I restored the varargin in setup_Stats. Current only the dot indexing errors are left. image

j-emberton commented 1 month ago

Some of them failed because of this error. It's due to the getfield function and seems unrelated to Stats. Others are due to the extra input arguments to setup_Stats in the test, so I restored the varargin in setup_Stats. Current only the dot indexing errors are left. image

Have you looked at the cpdAssign function? There's a section in it where it parses the stats2 data to extract certain fields. I think that's where it's failing.

j-emberton commented 1 month ago

@dorchard , did you say you had written some comments on this PR. I don't see any here.

dorchard commented 1 month ago

@dorchard , did you say you had written some comments on this PR. I don't see any here.

Sorry forgot to submit the review. Done now.

j-emberton commented 1 month ago

all tests pass

dorchard commented 3 weeks ago

I think we can merge this now.