search

astropy / SPISEA

Stellar Population Synthesis Modeling
https://spisea.readthedocs.io/en/stable/index.html
64 stars 32 forks source link

Resolved multiplicity #25

Closed nsabrams closed 4 years ago

nsabrams commented 4 years ago

Added resolved multiple systems to SPISEA and an example of how to use it popstar/resolved_multiples_example.py

mwhosek commented 4 years ago

Thank you for submitting this feature update, and for including documentation, test functions, and a jupyter notebook example! I think this will be a great addition to SPISEA. I had a few requests before we execute the merge:

1) Please submit the PR to the dev branch, rather than main. We'd prefer a workflow where features forks/branches are first merged into dev, and then we'll merge the changes into main as part of an official sub-release.

2) Please pull from the current dev branch in order to merge the software name change with your updates. For example, the popstar directory (which contains the .py files) has been replaced by spisea

3) Please move popstar/resolved_multiples_example.ipynb to the top-level docs directory, which is where the other jupyter notebook examples live. Note that you will need to update the import statements in order for the notebook to work with the new name.

4) Could you expand the documentation for multiplicity.MultiplicityResolvedDK so the input parameters for the init function (e.g. a_amp, a_break, a_slope1, etc) are explicitly defined? This way, our auto-documentation will pick it up and help other users better understand what the parameters mean.

An additional thought to consider:

What do you think about adding the functionality of synthetic.ResolvedCluster_ResolvedMult() a part of the parent synthetic.ResolvedCluster object? For example, perhaps you can take lines 573 -- 585 of your synthetic.py and insert it at line 172 in synthetic.ResolvedCluster. This way, any of the resolved cluster sub-classes will have the ability to have resolved multiplicity, and we avoid the failure mode where the user tries to use ResolvedCluster_ResolvedMult with a Multiplicity object that doesn't actually have resolved multiplicity.

In this setup, you would need to catch the case where the multiplicity object supplied to ResolvedCluster doesn't support resolved multiplicity (perhaps by adding a boolean flag variable to the init function of the parent MultiplicityUnresolved object that is False by default but is set to True for the MultiplicityResolvedDK object?). In those cases, you could return np.nans in the log_a, e, i, etc columns in the companion table, or simply not add those columns at all.

I'm open to suggestions and happy to discuss this further. I look forward to incorporating your contributions into SPISEA!

Thanks! Matt

nsabrams commented 4 years ago

Thanks for the comments. I'll make the changes and submit to dev.