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 edited #26

Closed
wants to merge 28 commits into from

Conversation

nsabrams
Copy link
Contributor

I made the suggested edits including putting the functionality of the ResolvedCluster_ResolvedMult object into ResolvedCluster.

nsabrams and others added 27 commits June 25, 2020 15:34
@mwhosek
Copy link
Contributor

mwhosek commented Jul 16, 2020

Looks great, thanks for implementing those changes! I was able to pull down your updated spisea code and everything appears to be working well.

I only have one final (and minor) comment, regarding your resolved_multiples_example.ipynb. I really like that you've included a jupyter notebook to demonstrate how it works, but I noticed that it has a dependence on the gcwork package. If a user doesn't have access to this package, then the first cell will fail. It doesn't look like you actually use the gcwork --> orbits in the notebook, so can it be removed?

Once this is addressed, we are ready to merge to dev.

class MultiplicityResolvedDK(MultiplicityUnresolved):
"""
Sub-class of MultiplicityUnresolved that adds semimajor axis and eccentricity information
for multiple objects from distributions described in Duchene and Krauss 2013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check, I think Kraus is just one s?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, Kraus is one "s"


Parameters
--------------
a_amp - float, optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standard commenting should be:

a_amp : float, optional
some comment here...

Change so auto-docs work.

@nsabrams
Copy link
Contributor Author

I'll make the changes and resubmit. As discussed with Matt, I'll change the structure of the resolved clusters example to be in two sections so there aren't issues with importing gcworks

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

4 participants