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

DM-37902: Mask edges at the AMP level as default when calculating the PTC #167

Merged
merged 2 commits into from Feb 16, 2023

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Feb 7, 2023

No description provided.

@plazas plazas requested a review from czwa February 8, 2023 02:50
@craiglagegit
Copy link
Collaborator

When I ran this, I also included isr:doSuspect=True, because the default is False. But I see you are doing it in the ptcExtract task instead of the isr task. So then the doSuspect=True is not needed, is that correct?

@plazas
Copy link
Contributor Author

plazas commented Feb 8, 2023

When I ran this, I also included isr:doSuspect=True, because the default is False. But I see you are doing it in the ptcExtract task instead of the isr task. So then the doSuspect=True is not needed, is that correct?

@craiglagegit We can add this to the isr part of the pipeline in the new yaml file I created. Why is this option needed?

@craiglagegit
Copy link
Collaborator

Well, maybe I am confused, but I thought the two options you added will mark the 20 pixels around the edge of each amp as "Suspect". but unless doSuspect=True, the Suspect pixels will not actually be masked. I could easily be wrong on how this works.

@czwa
Copy link
Contributor

czwa commented Feb 8, 2023

It would be good to run a quick test to confirm that doSuspect is or is not needed. I think on my read that it isn't (ptcExtract calls the IsrTask.maskEdges, and the SUSPECT bit is included in the ptcExtract maskNameList, so the edge should be excluded from the calculation on line 687-697), but I may be missing something subtle.
I'm marking this as Approved on the assumption I did trace through the code correctly (and that the test will be run to check).

@plazas plazas merged commit a5d8f4e into main Feb 16, 2023
@plazas plazas deleted the tickets/DM-37902 branch February 16, 2023 00:45
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.

None yet

3 participants