Dictionary parameters fix

[devforfu]

Here is an attempt to fix dictionary parameters issue #307 . Not sure if it covers all possible cases and the resulting dataframe with report contains frozen sets as keys. So probably there is another way to fix this issue in more clear way, i.e. to make more adjustments into batch processing code fragment.

[njvrzm]

I wonder if it would be better to make the arguments more explicit rather than making an exception for a specific set of types (Mapping types). With the current code the issue could be solved on the user's part by passing {"grid_params": [{"height": 10, "width": 10}]}. The PR avoids this annoyance for Mapping types but if you want a static parameter that is a list (or a tuple, or a set, or any other type with __iter__) you still need to know to wrap it in a list.

What if instead the BatchRunner took two arguments, static_parameter_values and variable_parameter_values, where the second argument is explicitly required to be a mapping of {<parameter>: <sequence of values>}?

[devforfu]

@njvrzm I guess you're right, it would be better. Actually, I was only thinking about case with mappings because directly faced with difficulties arising from absence of this functionality. And right now it looks quite cumbersome because it is not make too much sense to use static param in product() call.

Though there is still a question how to present results. I mean, the data frame that collects statistics uses passed arguments as keys. So there is also a question how to report this result with static params, i.e. is it worth to exclude them from data frame or maybe somehow "flatten" static argument to fit into data frame columns.

Anyway, I'll try to do it somehow.

[jackiekazil]

(sorry for the delay of my chime in... I have one more component to my Ph.D exam that wraps up today --- I will catch up after that.)

[jackiekazil]

Bringing this back up...
@devforfu the CI is failing. Can you do a flake8 check & fix the errors? Use the following command.
flake8 . --ignore=F403,E501,E123,E128 --exclude=docs,build

[devforfu]

@jackiekazil I think that maybe I should refactor this part in a way that @njvrzm proposed. So if you think it is worth to do, I can proceed as he suggested to make it more general.

[dmasad]

@devforfu If you're willing to take it on, I like @njvrzm's suggested way of doing it -- it seems cleaner, and makes what parameters are being explored more explicit.

[njvrzm]

I'd be happy to help out if you like, @devforfu. (Also it occurred to me while looking over this again that "fixed_parameter_values" is probably a better name than "static...".)

[devforfu]

@dmasad Ok, great. Agree, this approach is better then current hotfix.

@njvrzm Sure, much appreciated!

[devforfu]

Sorry for late response, here an updated version:
https://github.com/devforfu/mesa/blob/dict_param_fix/mesa/batchrunner.py

@njvrzm, do you think it is what you've meant? I've attempted to separate fixed and variable parameters and also handle case if the last argument is not a **kwargs. Not sure if it is needed thought.

[devforfu]

Also I though about something like scikit-learn approach where they have a separate ParametersGrid iterable that is used to configure grid search. Or maybe kind of set_params interface to setup models from outside.

[coveralls]

Coverage increased (+0.1%) to 75.336% when pulling a896511 on devforfu:dict_param_fix into c6e416b on projectmesa:master.

[devforfu]

Slightly adjusted runner and tests.

Also could someone tell me please, is it an intentional decision to use unittest with plain assert statements?

[coveralls]

Coverage decreased (-0.2%) to 75.076% when pulling 8fa2951 on devforfu:dict_param_fix into c6e416b on projectmesa:master.

[coveralls]

Coverage increased (+1.05%) to 76.284% when pulling f653738 on devforfu:dict_param_fix into c6e416b on projectmesa:master.

[dmasad]

@devforfu re assert statements, I think that was just going for the easiest-possible initial implementation. Is there a better practice you'd suggest?

[njvrzm]

Thanks for this and sorry I haven't had a chance to review it yet! I will do so this weekend.

[devforfu]

@dmasad That's right, asserts are quite clear way to express test cases I guess. The only issue that default Python test runner doesn't explain what is going wrong when assert failed.

class TestSomething(unittest.TestCase):

    def test_something_failed_with_plain_assert(self):
        x = 1
        y = 2
        assert x == y

    def test_something_failed_with_unittest_assert(self):
        x = 1
        y = 2
        self.assertEqual(x, y)

And here is an output:

======================================================================
FAIL: test_something_failed_with_plain_assert (__main__.TestSomething)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "...", line 8, in test_something_failed_with_plain_assert
    assert x == y
AssertionError

======================================================================
FAIL: test_something_failed_with_unittest_assert (__main__.TestSomething)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "...", line 13, in test_something_failed_with_unittest_assert
    self.assertEqual(x, y)
AssertionError: 1 != 2

You see that the last one, I mean, "dedicated" self.assertEqual, automatically substitutes actual values of variables into assertion output. And actually they already used in some places of codebase like self.assertRaises method. That's why I've asked about it.

And as a side note, probably you know, there is a pytest runner that allows to use assert statements but gives deeper inspection that default one (i.e. this guy dynamically replaces asserts with custom wrappers and shows more information about failed cases). It also has some other benefits, but maybe it is a matter of taste.

@njvrzm Sure, no problem, thank you!

[njvrzm]

I'm merging this into a new branch so I can do some cleanup before merging into master. I'll make a new PR for that shortly!