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

Fix powerspectrum bands on notebooks #527

Merged
merged 5 commits into from
Jan 11, 2025

Conversation

gabrevaya
Copy link
Contributor

This PR fixes the repeated generation of powerspectrum bands and labels in Jupyter notebooks and Literate caused by those notebooks making Makie compute the final limits twice. With this change, the bands and labels are created only the first time, and the following times only their bounds/positions are modified.

I also reduced the threshold for checking if the solution was saved at regular time steps to avoid spurious warnings.

@MasonProtter MasonProtter mentioned this pull request Jan 10, 2025
@harisorgn
Copy link
Member

The CI Basics errors have to do with this line

param_pairs = make_unique_param_pairs(params_affect)

where
function make_unique_param_pairs(params)
# HACK : MTK will complain if the parameter vector passed to a functional affect
# contains non-unique parameters. Here we sometimes need to pass duplicate parameters that
# affect states in the loop in LIF_spike_affect! .
# Passing parameters with Symbol aliases bypasses this issue and allows for duplicates.
param_pairs = if unique(params) == length(params)
[p => Symbol(p) for p in params]
else
map(params) do p
if count(pi -> Symbol(pi) == Symbol(p), params) > 1
p => Symbol(p, "_$(rand(1:1000))")
else
p => Symbol(p)
end
end
end
return param_pairs
end

I don't see how this is affected by this PR. I tried updating my local environment in case it was an upstream issue but the same tests pass for me.

@harisorgn
Copy link
Member

harisorgn commented Jan 11, 2025

Ok as suspected

p => Symbol(p, "_$(rand(1:1000))")

gave the same number twice. Passes after rerunning.

We could write so that it's impossible to hit duplicates but this is a quick (lazy) fix for now.

@gabrevaya
Copy link
Contributor Author

Great, all test passed! Should we merge it already?

@gabrevaya gabrevaya merged commit 6e30cf7 into master Jan 11, 2025
3 of 6 checks passed
@gabrevaya gabrevaya deleted the fix_powerspectrum_bands_on_notebooks branch January 11, 2025 14:18
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

Successfully merging this pull request may close these issues.

3 participants