-
Notifications
You must be signed in to change notification settings - Fork 12
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
Problems highpass filtering data with calibrated contrast curves #148
Comments
Relatedly, it looks like the kwargs for pyklip are read in from the database. I believe (but @semaphoreP can correct me) that the pyklip inputs are all saved to the pyklip headers, and so I wonder if it might be safer to read them all from there when things are loaded in again e.g. in line 529. |
Hi @maxwellmb. Thanks a lot for bringing up this issue! Please note that high-pass filtering is currently not supported (with the develop branch). We are working on a fix for this (see dev/jk branch). However, this will also require updates in the pyKLIP code, see comment here. We are planning to merge this fix as soon as the updates on the documentation have been completed. |
Max just made this PR to pyklip (https://bitbucket.org/pyKLIP/pyklip/commits/2138fc075f9fe201f4a8efbb3ac0a892514d9cea), @kammerje should check if this is the same behavior he is aiming for or different. (Basically, whether to filter the master library on construction, or at the klip_dataset level [which Max is proposing]) |
@semaphoreP Ok, I will check! Does this also apply the high-pass filtering to the KLIP FM dataset? |
Not currently. we would need to do the same thing here. I think we should just be consistent between the two of you, and not have 2 different sets of behaviors for how to high pass filter the reference library |
For now, I always applied the high-pass filter when generating the RDI library with pyKLIP. This ensures that PSF subtraction and FM behave consistently. Everything else is then spaceKLIP business, right? |
I think that makes sense, but is not the behavior @maxwellmb proposed in his pyklip PR, so we should see if he's ok with that |
Alright, I've implemented a version like Jens' suggestion, see the commit here. I tested it with a hacked up version of spaceklip of mine, not with dev/jk (since that requires a bunch of updating things for me), but it produces the desired behavior, including throwing an error if |
Yes, this is exactly what I had done too! |
Great! @kammerje should I accept Max's pyklip PR, or do you want to merge any additional changes you made into it? |
@semaphoreP Yes, please do so, this is exactly what I need for my spaceKLIP branch to work! |
Ok merged those changes in pyklip! |
@maxwellmb is this resolved now? |
If a user were to pass in the highpass keyword to pyklip, they might also want this to propagate to the calibrated contrast curve inject, but at the moment, on this line in Analysistools, highpass is hardcoded to false. Is there a reason for this or can we let it be the same is what was done in the PSF subtraction call to pyklip?
The text was updated successfully, but these errors were encountered: