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

Add default parameter handling for macro elasticity simulation #908

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

lucassz
Copy link
Contributor

@lucassz lucassz commented Jun 18, 2018

Add default parameter handling for macro elasticity simulation. Follow-up to #883 and #905: currently, running the macro elasticity model without inputting any data results in not even a UI error, but an uncaught server error in converting an empty string to a float. This adds default handling in a general way. However, it does make the assumption that there is only one column/value per input; this was already implied in the previous code, and I believe that should be okay for this model, but please let me know if it is not.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 18, 2018

@lucassz Nice catch. Would you mind adding a test in the dynamicapp where a reform is submitted with the defaults?

@lucassz
Copy link
Contributor Author

lucassz commented Jun 18, 2018

@hdoupe This is done. I had to make the class not be a TestCase to get the parametrization to work.


def assertTrue(self, a):
assert a

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lucassz what do you think about dropping the assert* methods? When I removed the TestCase inheritance from some of the other test classes, I dropped these methods because I think it's easier to just read a==b instead of self.assertEqual(a, b).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that is a better idea. I was trying to minimize changes to the code, but will make the change.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 20, 2018

LGTM. @lucassz are you ready for this to be merged?

@lucassz
Copy link
Contributor Author

lucassz commented Jun 20, 2018

@hdoupe Seems good to me too.

@hdoupe
Copy link
Collaborator

hdoupe commented Jun 20, 2018

Cool, thanks for the contribution.

@hdoupe hdoupe merged commit 72112f5 into ospc-org:master Jun 20, 2018
@lucassz lucassz deleted the macro branch July 2, 2018 15:38
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