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

Power spec #108

Merged
merged 17 commits into from
Aug 11, 2023
Merged

Power spec #108

merged 17 commits into from
Aug 11, 2023

Conversation

boryanah
Copy link
Collaborator

@boryanah boryanah commented Aug 8, 2023

Change including TSC offset and minor bugs for LCV

@boryanah
Copy link
Collaborator Author

boryanah commented Aug 8, 2023

@lgarrison updating the quick thing for CV, but ran into classy compile issues (and I think the other tests got canceled alongside that).

@lgarrison
Copy link
Member

There's a workaround for classy on main, can you merge main into this branch?

Fixing the classy issue
@boryanah
Copy link
Collaborator Author

boryanah commented Aug 8, 2023

Merged it, but there seems to be some issue now with Corrfunc -- not sure why it only shows up in this branch

@lgarrison
Copy link
Member

Looks like it was just a transient issue, re-running the jobs fixed it.

@lgarrison
Copy link
Member

@boryanah In using calc_power(), I realized we didn't have default values for most of the arguments, so I set some. Can you check that they look good? The only one that's not optional is Lbox.

This did have the side effect of changing the order of the arguments, so just be aware in case you have any code that's using calc_power() already (I changed all the places it was used in the repo).

@boryanah
Copy link
Collaborator Author

boryanah commented Aug 9, 2023

Thanks! The default values make sense for calc_power(). In describing the changes here, I forgot to say that ic_fields.py was optimized, too (everything now uses numba). There was also a mode counting bug, which doesn't matter in squaring k_x,y,z.

@boryanah
Copy link
Collaborator Author

boryanah commented Aug 9, 2023

I wonder if we should switch to passing k_bins and mu_bins directly in calc_power, but perhaps we can leave this for later.

abacusnbody/analysis/power_spectrum.py Show resolved Hide resolved
abacusnbody/analysis/power_spectrum.py Outdated Show resolved Hide resolved
abacusnbody/analysis/power_spectrum.py Outdated Show resolved Hide resolved
abacusnbody/analysis/tsc.py Outdated Show resolved Hide resolved
@lgarrison
Copy link
Member

I was thinking about changing the kbin/mubin arguments too, so I just went ahead and did it. Now they both accept either a number of bins or an array of bins.

@boryanah
Copy link
Collaborator Author

Ok, this looks great to me, so I'll just merge -- thanks.

@boryanah boryanah merged commit 5526d15 into main Aug 11, 2023
8 checks passed
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.

None yet

2 participants