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

Add sqrt_stretch and custom mask to align steps #183

Merged
merged 5 commits into from
Jun 18, 2024
Merged

Conversation

wbalmer
Copy link
Collaborator

@wbalmer wbalmer commented Jun 9, 2024

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.

@wbalmer
Copy link
Collaborator Author

wbalmer commented Jun 10, 2024

Bug with take_sqrt option in find_nircam_centers (now fixed)

@AarynnCarter
Copy link
Collaborator

@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!

@wbalmer
Copy link
Collaborator Author

wbalmer commented Jun 14, 2024

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

@AarynnCarter
Copy link
Collaborator

@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

else:
    img1 = datasub* masksub
    img2 = model_psf* masksub

But for anything else, we can adjust to the following:

if take_sqrt:
    img1 = np.sqrt(np.abs(datasub))* masksub
    img2 = np.sqrt(np.abs(model_psf))* masksub

to

if scale_exponent != 1:  #Or some other sensible name
    img1 = np.power(np.abs(datasub), scale_exponent)* masksub
    img2 = np.power(np.abs(model_psf), scale_exponent)* masksub

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?

@wbalmer
Copy link
Collaborator Author

wbalmer commented Jun 14, 2024

Sounds good! I'll test this now.

@wbalmer
Copy link
Collaborator Author

wbalmer commented Jun 14, 2024

Okay, tested in both the shft_exp = 1 and shft_exp = 0.5 cases and expected behavior was reproduced. Should be good to go

Copy link
Collaborator

@AarynnCarter AarynnCarter left a 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!

@wbalmer
Copy link
Collaborator Author

wbalmer commented Jun 18, 2024

Should be fixed!

@AarynnCarter AarynnCarter merged commit f7ffaba into develop Jun 18, 2024
2 of 4 checks passed
@wbalmer wbalmer deleted the stretch_align branch June 18, 2024 13:55
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.

2 participants