Skip to content

Conversation

@ihopethiswillfi
Copy link
Contributor

Fixes #466

@coveralls
Copy link

coveralls commented Mar 28, 2018

Coverage Status

Coverage decreased (-0.6%) to 80.594% when pulling 5a56c11 on ihopethiswillfi:master into 8ff0f7d on projectmesa:master.

@ihopethiswillfi
Copy link
Contributor Author

ihopethiswillfi commented Mar 28, 2018

Can anyone help write a test? I looked at it for a while but wasn't sure how to best approach it. I haven't written tests before :)

I was thinking to just copy the function at the bottom of test_batchrunner.py and set
self.variable_params = {}

Also I don't understand why the function test_model_with_fixed_parameters_as_kwargs doesn't seem to cover this. It seems to launch a batchrunner WITH fixed_params and WITHOUT variable_params , which is what we need to test.

"""
def __init__(self, model_cls, variable_parameters=None,
def __init__(self, model_cls, variable_parameters={},
Copy link
Member

@dmasad dmasad Apr 8, 2018

Choose a reason for hiding this comment

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

It's generally not a good practice to use a mutable value (e.g. {}) as a default argument. In this case I don't think anything harmful would happen, but I'd suggest leaving None as the default value just in case, and for general Pythonic-ness. Then you can add something like if not variable_parameters: variable_parameters = {}

@dmasad
Copy link
Member

dmasad commented Apr 8, 2018

It's been a while since I've looked at the test code, but it looks like in test_model_with_fixed_parameters_as_kwargs, the variable_parameters are still the default self.variable_params values set in the test default. To test not having any explicitly, you'd need to set self.variable_params = None.

@jackiekazil jackiekazil added help wanted Sprints! A task that might be good to tackle during sprints! and removed help wanted labels May 14, 2018
@jackiekazil jackiekazil mentioned this pull request May 14, 2018
@jackiekazil
Copy link
Member

@dmasad -- can you remind me / us where this left off? other than fixing conflicts? (I can't remember what as the explicit ask on this.)

@ihopethiswillfi
Copy link
Contributor Author

This is 'fixed' but the new code isn't covered by any tests. Unfortunately I haven't had the time to look into that and I don't think I'll have time in upcoming weeks either.

Also the variable_parameters={} should be taken care of as Dmasad already mentioned, which should be very simple to do.

@jackiekazil jackiekazil added this to the Kearny v0.8.5 milestone Oct 5, 2018
@dmasad
Copy link
Member

dmasad commented Oct 7, 2018

Turns out this is slightly more complicated than expected because BatchRunnerMP also needs to be updated to work with this. I'm working on it.

@dmasad dmasad mentioned this pull request Oct 9, 2018
@dmasad
Copy link
Member

dmasad commented Oct 9, 2018

Closing; superseded by #596

@dmasad dmasad closed this Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Sprints! A task that might be good to tackle during sprints!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants