kymata-atlas / kymata-core

Core Kymata codebase, including statistical analysis and plotting tools
https://kymata.org
MIT License
5 stars 0 forks source link

Gridsearch normalisation not working for language after the latest update #349

Open young-x-skyee opened 1 month ago

young-x-skyee commented 1 month ago

After merging the main branch into the language branch today, the warning messages seem to appear for too many times which did not happen before...

/imaging/projects/cbu/kymata/analyses/tianyi/kymata-core/kymata-core-data/output/fc2_test/decoder/log/slurm_log_4.txt image

young-x-skyee commented 1 month ago

The problem seems to be in the new vector.py file because when I only change that file back it is working again.

young-x-skyee commented 1 month ago

Just to clarify, I'm working on the kymata-language branch and the .sh file I'm using is

/imaging/projects/cbu/kymata/analyses/tianyi/kymata-core/submit_gridsearch_models_fc2_decoder.sh

neukym commented 1 month ago

And the commit hash for it is 64fcdadf0dd0759ade29dd388cbdf5658dd2f82f.

caiw commented 1 month ago

That x /= _normalize_magnitude(x) is inside a context manager which should raise an error on a divide-by-zero:

with np.errstate(divide="raise"):
    x /= _normalize_magnitude(x)

so I reckon this means it's dividing by nan, or perhaps inf. Looking at _normalize_magnitude(), it's just doing this:

def _normalize_magnitude(x: NDArray) -> NDArray:
     """Reusable magnitude function for use in `normalize`."""
     return np.sqrt(np.sum(x**2, axis=-1, keepdims=True))

So if it's producing a nan, it must be because the input contains a nan. Or it's possible that the input is too large and the sum-squared is going to inf. I did put in a gross hack which multiplies up the input by 10^6 in case it has zero magnitude (to avoid divide-by-zero errors on very small inputs to normalize(), where the magnitude can go to zero because of float16 precision), so if the input was very large, then multiplying by 10^6 could make it too big. But that should only happen if the input had a zero magnitude in one slice.

So if my reasoning is right (and it may not be!), it seems like the changes to vector.py should at most have exchanged some divide-by-zero warnings for some divide-by-nan/inf warnings...

@young-x-skyee Does the above shed any light on the issue? Does the emeg data you're loading in contain nans? Or perhaps it contains some dead channels which are all constant, and therefore can't be normalized without triggering a warning?

caiw commented 1 month ago

Having said the above, I'm now using full floats for function values, not float16s, so my awful hack might not be necessary any more. You could try commenting out

if (_normalize_magnitude(x) == 0).any():
    x *= 1_000_000

from normalize(). If that fixes it, please submit a pull request which deletes those lines and I'll test on the functions I was running which previously required it. If not, we can dig further into it.

caiw commented 1 month ago

Note to self: better yet, normalize should do something like x.astype(float) before calling _normalize_magnitude, to ensure the problem is always avoided. Need to think about how to deal with the inplace arg though.