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

Jitter Classes (#249) #253

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Jitter Classes (#249) #253

wants to merge 6 commits into from

Conversation

maxecharles
Copy link
Collaborator

PR to track changes.

Adding the ApplyAsymmetricJitter class I wrote for TOLIMAN, parametrised by r, phi, and shear.
I basically completely rewrote the generate_kernel method in a way that seems a bit more intuitive to me:

    def generate_kernel(self, pixel_scale: float) -> Array:
        
        ...

        # Generate distribution
        extent = pixel_scale * self.kernel_size  # kernel size in arcseconds
        x = np.linspace(0, extent, self.kernel_size) - 0.5 * extent
        xs, ys = np.meshgrid(x, x)
        pos = np.dstack((xs, ys))

        kernel = multivariate_normal.pdf(
            pos, mean=np.array([0.0, 0.0]), cov=self.covariance_matrix
        )

        return kernel / np.sum(kernel)

I kinda reimplemented this way of generating the kernel into the original ApplyJitter detector class.

surely i become a real dLux contributor SURELY

@maxecharles
Copy link
Collaborator Author

also i have NOT tested the new ApplyJitter

@maxecharles
Copy link
Collaborator Author

Spoke with @LouisDesdoigts and we (he) decided that the ApplyJitter class was made entirely redundant by the new ApplyAsymmetricJitter class, so we have now replaced ApplyJitter with ApplyAsymmetricJitter. So there's no more ApplyAsymmetricJitter anymore but now ApplyJitter is ApplyAsymmetricJitter if you get what I'm saying

@benjaminpope
Copy link
Collaborator

Could even just rename the asymmetric jitter class to the new ApplyJitter...

@LouisDesdoigts
Copy link
Owner

Yeah @maxecharles made a typo in his comment (GOT HIM), its just going to be called ApplyJitter

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.

3 participants