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

Resolved multiplicity #25

Closed
wants to merge 20 commits into from
Closed

Conversation

nsabrams
Copy link
Contributor

@nsabrams nsabrams commented Jul 9, 2020

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

@mwhosek
Copy link
Contributor

mwhosek commented Jul 14, 2020

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
Copy link
Contributor Author

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

@nsabrams nsabrams closed this Jul 14, 2020
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.

3 participants