Add warnings for conflicts between fixed params and param grids #297

mulhod · 2016-02-13T05:25:32Z

Added warnings for conflicts between fixed parameters and parameter grids and for when grid search is set to False and paramater grids are specified.

…arnings in config.py so that user will be warned of conflicts and if grid_search is False and param_grids/fixed_parameters are specified (or at least one of them)

desilinguist · 2016-02-13T16:14:45Z

skll/config.py

+                               'cases.'.format(learner,
+                                               ', '.join(overlap_params)))
+    else:
+        if param_grid_list or fixed_parameter_list:


What's the problem with specifying fixed_parameter_list even if we aren't doing grid search?

…en_fixed_params_and_param_grids_185

desilinguist · 2016-02-18T16:44:03Z

Okay, @aoifecahill and I talked about this a bit and we think this is a little bit tricker:

If users specify a parameter in fixed_params but also specified the same parameter as part of their parameter grid to search, that should actually be an error since it's not clear what their intent is.
However, if users specify a parameter in fixed_params but have only specified grid_search=True and the default parameter grid happens to include said parameter, there should be a warning saying that the fixed parameter will be ignored. But that warning should also tell the user how to avoid this if they want - by specifying a parameter grid explicitly which does not include this parameter.
We should also update the documentation for fixed_params to indicate all of the above.

Thoughts?

mulhod · 2016-02-18T16:57:25Z

Seems pretty clear to me. I'll update this shortly.

dan-blanchard · 2016-02-18T17:08:45Z

However, if users specify a parameter in fixed_params but have only specified grid_search=True and the default parameter grid happens to include said parameter, there should be a warning saying that the fixed parameter will be ignored. But that warning should also tell the user how to avoid this if they want - by specifying a parameter grid explicitly which does not include this parameter.

To me it would make the most sense in this case for the fixed parameter to win out over the default parameter grid. We could just modify the grid for them automatically and likely get them what they wanted.

desilinguist · 2016-02-18T17:43:04Z

Hmm, yeah I agree. That's a better way of doing it but we should still output a warning saying that we did that.

dan-blanchard · 2016-02-18T17:55:08Z

Yeah, I agree about the warning.

mulhod · 2016-02-19T05:20:02Z

Couple observations:

First, I noticed an error in the check the way I implemented it and it's been fixed: I was zipping some things together and got the sequence of the zipping wrong. So, I just wanted to mention this in case it's been noticed.
More importantly, I have realized that the original way I was doing this wasn't correct. I was treating it as if param_grid_list would be populated with the default values before the point when it would get to the check at the center of this PR. However, the default values only get substituted in later on if param_grid_list either just wasn't specified at all or if some of the parameter grids in it are equal to an empty list, in which case it is my understanding that the default parameter grid is used. This was written this way in order to make it so that users could specify some, but not all parameter grids. Having an empty list was the same as saying, "Use the default grid". Given that the value of param_grid_list will either be None (if not specified) or a list of parameter grid dictionary lists (which might contain some empty lists), my check here will have to be a lot more involved -- if fixed_parameters and grid_search are specified. My check will basically need to look at the default values and see if there are conflicts and, in some situations, actually override values. And, since the would have happened after this point the way things were, it will now happen here.

…lues; make new unit test to test for conflicts when both are specified in the configuration file (more unit tests to come soon)

… parameter grids can be attained and add a new unit test in (more to come later)

…en_fixed_params_and_param_grids_185

…ing bad learner names themselves

…ters

mulhod · 2016-02-19T16:11:20Z

Ok, I've made a bunch of changes and I think this is finished -- or, at least, I don't want to work on this further until somebody else takes a look because I had to change quite a few things and I want to know first if this is what you had in mind. As I mentioned earlier, in order to intelligently resolve conflicts, logging warnings sometimes, raising exceptions others, or doing some things silently, the approach had to be much more involved than it was initially. Partially this is due to the scope of the PR changing a little after further discussion. Anyway, tests pass locally, so I'm fairly certain that it works as is.

desilinguist · 2016-02-19T16:20:01Z

doc/run_experiment.rst

-(and the defaults will be used).
+(and the defaults will be used). If there is a conflict between parameters
+specified in :ref:`param_grids` and :ref:`fixed_parameters`, :ref:`param_grids`
+values will take precedence.


Isn't this the opposite of what we finally agreed on (after @dan-blanchard's comment)?

I haven't dealt with updating the documentation yet after making the most recent changes and I'd prefer to get the greenlight on what I have so far before making those changes.

Ah, okay. I just wanted to make sure that the code has been updated to do the right thing and it's just the documentation that's out of sync.

…heck in config.py

mulhod · 2016-02-19T17:05:17Z

So, it seems that some of my tests are failing, but not the ones that are really for the core of this PR. Specifically, they are the tests for the learner names in the config file. I thought that I could simply tests all the input names and then raise an exception if they weren't in the list of learner classes. But, I wasn't accounting for custom learners. What I'm wondering is: did you not have this type of check specifically to allow custom learners? I could change my exception to a warning that simply says that the user is using an unrecognized/custom learner. Or I could take it out. Also, the reason I had added that in there was because I needed to be able to refer to a learner's corresponding class (without using eval) to get its default parameter grid. I could change that check below to look for the parameter grid and if none is found (i.e., because it's a custom learner), then obviously don't do the usual check since there will be no parameter grid.

desilinguist · 2016-02-19T23:05:31Z

Yeah, I think we weren't checking the learners's identity because people could write custom learners. I think this PR requires a bit of careful reviewing since the scope has expanded. I would like all of us to think about this carefully and reach a consensus. So, this can probably go into either 1.2.1 or 1.3, whatever release we do next.

mulhod · 2016-02-23T16:35:18Z

The check for a valid learner type, which my changes fail on due to the fact that users can provide custom learners, could actually be resolved just simply by checking if the learner ends with ".py", couldn't it? I think the error that I have put in could be kept without much modification.

I see now that it's a little more complicated. I thought that the learner name itself ended in .py for some reason, but that's only true for the custom_learner_path field in the config file. In any case, there might still be a way to check if that field is specified and just skip the check for invalid learners. Then, later on when we're trying to get the default parameter grids, we can just skip the check for any learners that aren't found in the list of possible learner types under the assumption that they are custom learners.

…en_fixed_params_and_param_grids_185

coveralls · 2017-11-27T15:54:13Z

Coverage decreased (-0.1%) to 92.236% when pulling eed801c on feature/warning_needed_for_conflicts_between_fixed_params_and_param_grids_185 into 112cd33 on master.

…nd paramater grids

coveralls · 2017-11-27T17:36:59Z

Coverage decreased (-0.03%) to 92.345% when pulling 6d82e56 on feature/warning_needed_for_conflicts_between_fixed_params_and_param_grids_185 into 112cd33 on master.

mulhod · 2017-11-27T18:17:24Z

The coverage decreased, but all that was added (now that some things have been removed) are basically logging statements. Tests could be added to check that the statements are logged, but I'm wondering if it's necessary or more trouble than it's worth in this case.

aoifecahill · 2017-11-27T18:18:42Z

wouldn't we want to check that the correct warnings are being logged for the various scenarios?

mulhod · 2017-11-27T18:22:16Z

It would be good, sure, but there are no checks for any of the other logging statements in similar situations. Also, it seems to me that it would be better if all of the warnings were actual warnings (that could be written to the log as well). That would be easier to test.

aoifecahill · 2017-11-27T18:25:50Z

Ah yes, true. But since PR #380 has been merged, shouldn't that now be possible? i.e. using proper logging and verifying the logging messages?

mulhod · 2017-11-27T18:27:53Z

Yeah, I see the case where it was used to verify the number of folds, etc. I think it's possible. I just wonder about 1) how worthwhile it is and 2) whether the warnings should be more like actual warnings. I can get around to adding some tests using the log statements for now.

aoifecahill · 2017-11-27T18:44:30Z

Yes, I agree that the warning should be more like actual warning following the logging changes that @desilinguist implemented recently.

coveralls · 2017-11-28T17:12:32Z