-
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
Add sqrt_stretch and custom mask to align steps #183
Conversation
Bug with take_sqrt option in find_nircam_centers (now fixed) |
@wbalmer, looks great! I did have one suggestion that could be a nice adjustment. Instead of a binary do/do not do a square root. Could we instead provide an exponent to scale the data by? The square root worked in your case, but others might find in the future that something like a cube root could be more useful, or who knows, even an exponent >1. With this, we can set the default exponent==1, and can cut down on some of the added if statements. Let me know what you think! |
Hi Aarynn. I see your point about generalizing/simplifying the statement. I suppose the problem is that np.power, the function I'd use to generalize this, returns nans when negative values (which there will be in median subtracted data) are raised to non integer exponents (like 1/2). np.sqrt and np.cbrt provides an interface for returning the principle root. So to implement something like you describe, I feel like it would end up being about as many if/else statements as there are currently (a case for exp=1/2 or 1/3 and then a generalized case for exp>=1). I can do this, if you'd still like to see the generalization |
@wbalmer Okay gotcha, in that case maybe we can strike a middle ground. For the nominal case (exp=1), we could use your current else statement
But for anything else, we can adjust to the following:
to
I feel like this keeps us to the current number of if statements, but more general, and avoids using the absolute function for the nominal case. For exponents >1, it won't matter that we've taken the absolute value as the result will be the same. Do you think that would make sense? |
Sounds good! I'll test this now. |
Okay, tested in both the shft_exp = 1 and shft_exp = 0.5 cases and expected behavior was reproduced. Should be good to go |
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.
Just one tweak, but otherwise looks good!
Should be fixed! |
See discussion in #114 . For data with more complex speckle patterns (e.g. NIRCam bar NARROW coronagraphy), it can be necessary to emphasize the speckle pattern itself rather than the central leakage term. I've found a combination of two strategies reproduces the commanded shifts from the SGD: masking the central leakage using a custom mask during the
align_frames
step, and taking the square root of the images before cross correlation. This PR introduces non-intrusive toggles to do both of these things.