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.

Sign up for GitHub

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

Jump to bottom

Run THELI on shapepipe part 1 #617

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Run THELI on shapepipe part 1 #617

wants to merge 11 commits into from

Conversation

lbaumo
Copy link

@lbaumo lbaumo commented Feb 23, 2023 •
edited by martinkilbinger
Loading

Summary

  • adds a bash script to convert THELI output to shapepipe input
  • changes bookkeeping in get exposures module
  • merge headers module now accepts external scamp headers
  • wcs no longer switches from PV to SIP, keywords are automatically changed in split exposure module, closes issue Remove the SIP to PV conversion #612
  • no longer explicitly sets weights to 1 after rescaling during shape measurement

Reviewer Checklist

Reviewers should tick the following boxes before approving and merging the PR.

  • The PR targets the develop branch
  • The PR is assigned to the developer
  • The PR has appropriate labels
  • The PR is included in appropriate projects and/or milestones
  • The PR includes a clear description of the proposed changes
  • If the PR addresses an open issue the description includes "closes #"
  • The code and documentation style match the current standards
  • Documentation has been added/updated consistently with the code
  • All CI tests are passing
  • API docs have been built and checked at least once (if relevant)
  • All changed files have been checked and comments provided to the developer
  • All of the reviewer's comments have been satisfactorily addressed by the developer

@lbaumo lbaumo self-assigned this Feb 23, 2023
@lbaumo lbaumo changed the title Weights Run THELI on shapepipe part 1 Feb 23, 2023
@martinkilbinger
Copy link
Contributor

Add the label for the PR. Otherwise looks good.

martinkilbinger
martinkilbinger requested changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@martinkilbinger martinkilbinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set the label of the PR.

aguinot
aguinot reviewed Mar 8, 2023
View reviewed changes
shapepipe/modules/ngmix_package/ngmix.py
@@ -813,7 +813,6 @@ def do_ngmix_metacal(

weight_map = np.copy(weights[n_e])
weight_map[np.where(flags[n_e] != 0)] = 0.
weight_map[weight_map != 0] = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create an issue. Line 859 there is weight_map *= 1 / sig_noise ** 2 if the weight_map is not equal to 1 it will lead to weird behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for theli weights, there are relative weights between pixels, which then need to be rescaled by the sky background variance to become variance maps. Thomas Erban seemed to think this operation was okay. are you talking about the weirdness associated with some megaprime weight maps that are not always 1 or 0?

@lbaumo lbaumo added the enhancement New feature or request label Apr 5, 2023
@lbaumo lbaumo added this to the Re-run 2023 milestone Apr 6, 2023
@sfarrens sfarrens removed this from the P3 re-run 2023 milestone Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aguinot aguinot aguinot left review comments

@martinkilbinger martinkilbinger martinkilbinger approved these changes

@sfarrens sfarrens Awaiting requested review from sfarrens

Assignees

@lbaumo lbaumo

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lbaumo @martinkilbinger @aguinot @sfarrens