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

BUg in do_combine_turb #49

Open
jbecherer opened this issue May 29, 2019 · 13 comments
Open

BUg in do_combine_turb #49

jbecherer opened this issue May 29, 2019 · 13 comments

Comments

@jbecherer
Copy link
Collaborator

Hi Deepak, how are things.
I realized a nasty bug in do_combine_turb.m
it is related to the noise_floor_mask and potentially other masks, too.
When I put noise_floor_fill_value to anything other than nan
line 230 [chi, chi.stats.spec_floor_percentage] = ApplyMask(chi, ...
is going to revive values that have been flagged before as nans to
the noise_floor_fill_value.
This is a big issue since it completely messes with the flagging.

@dcherian
Copy link
Member

It will replace previous NaNs with the noise_floor_fill_value when spec_area is close to noise floor, correct?

Isn't that what you want? I found recently that we usually get fits with 1-2 points in the fitting range when you're close to the noise floor...

@jbecherer
Copy link
Collaborator Author

Maybe its an easy fix. I think the problem is the way you define the criteria:
230 [chi, chi.stats.spec_floor_percentage] = ApplyMask(chi, ...
231 spec_floor_mask & isnan(chi.chi), '=', 1, ...
232 'spec_floor', [], CP.noise_floor_fill_value);

I think it should be simply
spec_floor_mask & **~**isnan(chi.chi)

can you confirm?

@jbecherer
Copy link
Collaborator Author

In the old version it overwrites old nans from other masks with the fill value, which is bad I think

@dcherian
Copy link
Member

I actually removed that at some point so it's just spec_floor_mask.

Again, if you are at the noise floor, you want to fill those values regardless of whether they were flagged earlier? Which other mask is this interfering with?

@jbecherer
Copy link
Collaborator Author

for example the Tz<minTz ...
pretty much all the other. I think.
you definatly don't want to set eps= 0
if noisefloor hit but Tz=0

@dcherian
Copy link
Member

the T_z mask is applied last to avoid this...

(also I made a local change to avoid setting ε=0. really I think we should only set χ=0 but leave ε as NaN. I can push that soon if that would be useful.)

@dcherian
Copy link
Member

I guess it interferes with N2 mask. Some reordering would fix that...

@jbecherer
Copy link
Collaborator Author

but also with speed masks....
not with Tz maks (you are right)
was you intent to have it recover previous nan values to zero?

@jbecherer
Copy link
Collaborator Author

or was it supposed to be an extra mask?

@dcherian
Copy link
Member

was you intent to have it recover previous nan values to zero?

Yes. Mostly because the n_freq mask usually nans out things that are close to noise floor i.e. that's mostly when we get bad fits.

I see your point though. We should reorder the masks so that we deal with bad fits, noise floor first and then do the rest.

Just to be sure that we're talking about the same thing; here's the output from the most recent version:

----------> adding mm1

    WARNING: min_N2 1.0e-06 < minimum resolvable based on SBE-37 accuracy specifications i.e. 1.1e-06
    5.5754% chi estimates are NaN before I truncate to valid time limits.
    5.5783% chi estimates are NaN before I start masking.

    ==== Now masking 1 sec averaged estimate.
    N2 < 1.0e-06 NaN-ed out 4.56% of estimates (1412983 points)
    inst speed (pumping + background) < 5.0e-02 NaN-ed out 0.00% of estimates (131 points)
    background flow < 5.0e-02 NaN-ed out 5.27% of estimates (1632951 points)
    max_chi > 1.0e-03 NaN-ed out 0.00% of estimates (410 points)
    max_eps > 1.0e-02 NaN-ed out 0.71% of estimates (219305 points)
    number of points in final fitting range, n_freq < 3.0e+00 NaN-ed out 4.34% of estimates (1344507 points)
    IC fit for a VC estimate, kstop < ki = 1.0e+00 NaN-ed out 0.57% of estimates (177003 points)
    Deglitch log10(chi)...
deglitch removed 78948 points = 0.3 % | avg(NaNed points) = -6.8e+00 | median(NaNed points) = -6.5e+00
    Deglitch log10(eps)...
deglitch removed 20913 points = 0.1 % | avg(NaNed points) = -6.1e+00 | median(NaNed points) = -6.0e+00
Elapsed time is 4.668614 seconds.
    Is Tp at noise floor? spec_floor_mask = 1.0e+00 NaN-ed out 19.84% of estimates (6141360 points)
    =================================================================
    .... Processing Winters & D'Asaro estimate. Takes 120s for 1 year.
      WDA: 3881 = 7.5178% time intervals have chi=NaN. 
      WDA: 26 = 0.050364% time intervals did not see enough pumping to make an estimate. 
Elapsed time is 13.541410 seconds.
    =================================================================
0.00% of WDA estimates are NaN because the chipod wasn't pumped enough.
    Tz < 1.0e-03 NaN-ed out 21.23% of estimates (6571325 points)
    undo NaNing of previously zero values = 1.0e+00 zeroed out 0.00% of estimates (0 points)

    Running moving average
    Summing up masks...
Elapsed time is 2.851035 seconds.

@dcherian
Copy link
Member

undo NaNing of previously zero values = 1.0e+00 zeroed out 0.00% of estimates (0 points)

This was a local change that I should get rid off. You wont' see this.

@jbecherer
Copy link
Collaborator Author

I was still on the old version, I didn't realize that you made so many changes recently.
The out put figures look way better, now:) not sure what everything means though :)
I think the issue remains though, right?

@dcherian
Copy link
Member

Yes, I think we should reorder the masks more sensibly.

The n_freq mask should come before the noise floor mask basically. Everything else can come later I think,

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

2 participants