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

tests in the Lognormal class to make sure mu and sigma have the same … #799

Merged
merged 2 commits into from
Aug 20, 2020

Conversation

sbenthall
Copy link
Contributor

This PR adds a couple exception handlers to the Lognormal class to check to make sure that if mu or sigma is a list, they are both lists, and are of the same length.

  • Update CHANGELOG.md with major/minor changes.

@sbenthall sbenthall requested a review from MridulS August 19, 2020 15:38
"mu and sigma must be of same length, are %d, %d" % (
len(list(self.mu)), len(list(self.sigma))
))

Copy link
Member

@MridulS MridulS Aug 19, 2020

Choose a reason for hiding this comment

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

what should happen if there are not lists?

good to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like, if they are scalars? Then they should be converted into length-1 lists, I guess ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

The internal representation should be numpy arrays, not lists. If the user provides a one-dimensional input, mu and sigma, those should be converted to two 1-element np arrays.

These topics should be moved to a different issue. @MridulS or @sbenthall will create the new issue

@llorracc llorracc added the Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged label Aug 20, 2020
@sbenthall sbenthall merged commit 20df134 into econ-ark:master Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-To-Merge Has been reviewed and when branch is updated and checks pass it should be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants