Skip to content

Added generalized multiple outputs - #510

Open
seiyatsukamoto wants to merge 1 commit into
ML4GW:regress_merger_timefrom
seiyatsukamoto:regress_merger_time
Open

seiyatsukamoto wants to merge 1 commit into
ML4GW:regress_merger_timefrom
seiyatsukamoto:regress_merger_time

Conversation

@seiyatsukamoto

Copy link
Copy Markdown

Changed the export and infer to map multiple outputs.
Changed Ledger to accept dictionaries.
Changed EventSet to take in extra_params.
Added median pool for regression outputs.
Changes shouldn't require changes for single output configs.
Changes were checked with timedomain, merger time regression, and multitask branches.

@wbenoit26

Copy link
Copy Markdown
Contributor

I'll give this a look tomorrow. Could you explain the motivation for these changes?

@seiyatsukamoto

Copy link
Copy Markdown
Author

It's for #474.
I also think that saving multiple outputs should be somewhere for the BBH + BNS runs.

@wbenoit26

Copy link
Copy Markdown
Contributor

I think we can simplify the ledger changes quite a bit by subclassing EventSet rather than adding dictionaries and extra params:

class MergerTimeEventSet(EventSet):
    merger_time: np.ndarray = parameter()

We'll have to generalize the recover function, but that should be doable.

There's a couple of bugs I've noticed in the base branch as well that we should get fixed. Here's what I suggest:

  1. I'll make a quick PR fixing those bugs.
  2. You rebase on top of that commit with the updated Ledger approach.
  3. After merging this in, we make a PR against dev to see what it would take to get things merged. We should do a complete test before merging things in, but it would be good to see the diff.

@wbenoit26

Copy link
Copy Markdown
Contributor

Just merged #511. Give the subclassing approach a try and rebase on top of it.

@seiyatsukamoto
seiyatsukamoto force-pushed the regress_merger_time branch 2 times, most recently from 0a353c9 to 8c70256 Compare October 5, 2026 20:58

This branch has not been deployed

No deployments
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