Skip to content

Conversation

@Corvince
Copy link
Contributor

@Corvince Corvince commented Nov 9, 2017

Added ability to include fixed parameters in get_model_vars_dataframe() function. See #422

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 81.929% when pulling a0b0158 on Corvince:BatchrunFixedParameters into 275674d on projectmesa:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 81.929% when pulling a0b0158 on Corvince:BatchrunFixedParameters into 275674d on projectmesa:master.

@TaylorMutch
Copy link
Contributor

@Corvince is there any inherit issue with the fixed_parameters being included by default, and have folks opt out of automatically including the fixed parameters? And if the someone doesn't include fixed_parameters as part of the BatchRunner's constructor, then there's no difference.

Also, since _prepare_report_table is a 'private' method anyway, could we instead have the BatchRunner class instead have a method that knows whether to include the fixed parameters or not?

@TaylorMutch TaylorMutch self-requested a review December 3, 2017 05:22
@TaylorMutch
Copy link
Contributor

@Corvince would the following approach achieve the same effect?

In BatchRunner.__init__:

...
self.fixed_parameters = fixed_parameters or {}
self._include_fixed = len(self.fixed_parameters.keys()) > 0
...

In BatchRunner._prepare_report_table:

...
if self._include_fixed:
        for param in self.fixed_parameters.keys():
        ordered[param] = self.fixed_parameters[param]
return ordered
...

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.09%) to 82.911% when pulling 59edb4e on Corvince:BatchrunFixedParameters into 63f3fbd on projectmesa:master.

@Corvince
Copy link
Contributor Author

Corvince commented Dec 6, 2017

Thanks @TaylorMutch. That seems to me like a better approach and achieves the same result. I think it makes sense to have fixed parameters in by default.

Copy link
Contributor

@TaylorMutch TaylorMutch left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@TaylorMutch TaylorMutch merged commit d263319 into projectmesa:master Dec 6, 2017
Corvince added a commit to Corvince/mesa that referenced this pull request Dec 21, 2017
* Added include_fixed parameter

* New approach that defaults to include fixed parameters

* Fixed test
@jackiekazil jackiekazil added this to the Hayden milestone Jan 6, 2018
@Corvince Corvince deleted the BatchrunFixedParameters branch June 14, 2019 03:08
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