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

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

Open
young-x-skyee opened this issue Jul 29, 2024 · 6 comments
Assignees
Labels
🪲 bug Something isn't working ❓ discussion needed Extra discussion is needed before work can commence gridsearch Related to the gridsearch

Comments

@young-x-skyee
Copy link
Contributor

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 young-x-skyee added 🪲 bug Something isn't working ❓ discussion needed Extra discussion is needed before work can commence gridsearch Related to the gridsearch labels Jul 29, 2024
@young-x-skyee
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

neukym commented Jul 29, 2024

And the commit hash for it is 64fcdad.

@caiw
Copy link
Member

caiw commented Jul 30, 2024

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
Copy link
Member

caiw commented Jul 30, 2024

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
Copy link
Member

caiw commented Jul 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 bug Something isn't working ❓ discussion needed Extra discussion is needed before work can commence gridsearch Related to the gridsearch
Projects
None yet
Development

No branches or pull requests

3 participants