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

Allow pippin to merge PROBIA designations #91

Closed
bap37 opened this issue Jun 15, 2022 · 3 comments
Closed

Allow pippin to merge PROBIA designations #91

bap37 opened this issue Jun 15, 2022 · 3 comments

Comments

@bap37
Copy link

bap37 commented Jun 15, 2022

Morning! (or evening)

This is admittedly as much of an SNANA issue as it is a pippin one, but I think the pippin-side fix is easier.

So for SALT2mu, when using photometric samples, SNANA requires a varname_pIa key to work. This varname_pIa key typically corresponds to the output of a classifier. And that's great for a single sample!

But when combining samples, such as SDSS and PS1, this causes it to immediately crash, since I can either specify the SDSS classifier or the PS1 classifier, but not both.

What I've been doing in the interim is waiting for the 6_BIASCOR step to crash, running a script to rename probabilities in 5_MERGE, then rerunning by hand after copying all the hash.txt values (because otherwise things will get overwritten and it'll crash again!).

While this works, it's a bit ugly, and I think this is gonna end up being a significant issue in the future as we move more towards photometric samples!

@RickKessler
Copy link
Collaborator

I just learned about this issue; please point to output on disk (or restore if it was clobbered).

@OmegaLambda1998
Copy link
Member

Proposed solution:

We add a flag to MERGE called MERGE_CLASSIFIERS which is a list of the classification tasks you want to merge. Then, instead of PROB_PROBIA_SDSS and PROB_PROBIA_PS1 pippin will see that the merged name will be PROB_PROBIA as that is the name with the most "overlap".
I think the cleanest way of providing this will be:

MERGE:
   MERGE:
      MERGE_CLASSIFIERS:
        - [SNN_PS1, SNN_SDSS]
        - [SCONE_X, SCONE_Y]

So that you can group classifiers together.
Then in BIASCOR you can either provide a similar list (CLASSIFIER: [SNN_PS1, SNN_SDSS]) or pippin can automatically notice that the classifier has been merged (CLASSIFIER: SNN_PS1), either way pippin will use the same logic as in MERGE to set varname_pIa to PROB_PROBIA. This way if you have multiple BIASCOR tasks you could specify classifiers which are part of a merge or not as long as the merge output you point BBC to has the right columns

I think I prefer this for a few reasons.

  1. It only adds one new flag, and it's a very simple flag
  2. You only need to provide Pippin objects without any knowledge of SNANA flags like varname_pIa
  3. It leaves a lot of the heavy lifting to Pippin

OmegaLambda1998 added a commit that referenced this issue Jul 1, 2022
… multiple classifers to point to the same probability column.
OmegaLambda1998 added a commit that referenced this issue Jul 1, 2022
Issue #91: Implemented a MERGE_CLASSIFIER key to AGGREGATION to allow…
@OmegaLambda1998
Copy link
Member

Resolved via f8cfb2d

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

No branches or pull requests

3 participants