-
Notifications
You must be signed in to change notification settings - Fork 2
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
basic powerspectra code + example #4
Conversation
code/utils/ps.py
Outdated
W[...] = 2.0 | ||
W[..., 0] = 1.0 | ||
W[..., -1] = 1.0 | ||
kedges = np.arange(kmin, np.pi * 10 / 1 + dk/2, dk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should go to higher k here... not sure if we want to have a kmax or similar option here...
code/utils/ps.py
Outdated
dig = tf.Variable(dig,dtype=tf.int32) | ||
Nsum = tf.Variable(Nsum,dtype=tf.complex64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these really have to be tf.Variables, but things work if they are tf variables? shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I could spend like 5 minutes to make this whole part differentiable too in case someone ever wants to take the derivative with respect to box size :P
I've looked at this PR, it looks pretty good :-) I'll just wait until we have code to do the CIC painting, so that we can test computing the powerspectrum directly from a TF field. |
Ok, so I've used this stuff in this demo notebook: It seems to work :-) but I have a few comments:
This way, it follows a structure similar to halotools, and then people would use with:
|
Okay! I fixed the kmax issue, and added a very trivial test (pk of noise) which I compared vs. nbodykit's powerspectra. I wasn't sure if we wanted to add a requirement for flowpm in order to do a slightly more nuanced test involving a flowpm mesh object with a real density field. |
Awesome :-D I'm happy to leave flowpm out of the validation of the power spectrum. waiting to see if we get the green light from github CI :-) |
Ok fantatstic! Let me just try to massage the PR into a cleaner version.... |
@bhorowitz here is the magic I used to clean up the PR and only show your modifications in the list of changed files:
That simplifies the git history, and only keeps the modifications that differ from the current master branch. More info here: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request#perform-a-rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ok Ben, it's ok for me if you want to press the big Green Merge Button, go ahead ;-) |
I've put up a pull request to add @bhorowitz! 🎉 |
A little tf implementation of a powerspectra estimator (more or less copied from nbodykit's projected PS implementation)... It seems to match that from nbodykit in the couple realizations/boxsizes I chose. It currently doesn't go up to as high of k as nbodykit by default; probably something to do with nyquist frequency definitions.
The IO for the pk function is very rough; depending on how we end up defining other functions perhaps there could be some standardization such as wrapping up all ps-related functions (sigma8? HMF?) in some class.