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

Problems highpass filtering data with calibrated contrast curves #148

Open
maxwellmb opened this issue Feb 10, 2024 · 13 comments
Open

Problems highpass filtering data with calibrated contrast curves #148

maxwellmb opened this issue Feb 10, 2024 · 13 comments
Labels
enhancement New feature or request

Comments

@maxwellmb
Copy link
Collaborator

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?

@maxwellmb maxwellmb added the enhancement New feature or request label Feb 10, 2024
@maxwellmb
Copy link
Collaborator Author

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.

@kammerje
Copy link
Collaborator

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.

@semaphoreP
Copy link
Collaborator

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])

@kammerje
Copy link
Collaborator

@semaphoreP Ok, I will check! Does this also apply the high-pass filtering to the KLIP FM dataset?

@semaphoreP
Copy link
Collaborator

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

@kammerje
Copy link
Collaborator

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?

@semaphoreP
Copy link
Collaborator

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

@maxwellmb
Copy link
Collaborator Author

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 klip_dataset is called with a different highpass value than what was used when reading in the rdi dataset. @kammerje, does this meet your needs?

@kammerje
Copy link
Collaborator

Yes, this is exactly what I had done too!

@semaphoreP
Copy link
Collaborator

Great! @kammerje should I accept Max's pyklip PR, or do you want to merge any additional changes you made into it?

@kammerje
Copy link
Collaborator

@semaphoreP Yes, please do so, this is exactly what I need for my spaceKLIP branch to work!

@semaphoreP
Copy link
Collaborator

Ok merged those changes in pyklip!

@AarynnCarter
Copy link
Collaborator

@maxwellmb is this resolved now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants