Sitelet https://github.com/EducationalTestingService/skll/pull/619
Skip to content

Simplify parameter grids - #619

Merged
desilinguist merged 6 commits into
masterfrom
618-simplify-default-param-grids
Jun 5, 2020
Merged

desilinguist merged 6 commits into
masterfrom
618-simplify-default-param-grids

Conversation

@desilinguist

Copy link
Copy Markdown
Collaborator

This PR closes #618.

  • Change the default parameter grids to be plain dictionaries instead of lists with a single dictionary.
  • Update docstrings and documentation to make it more consistent and compatible with this change.
  • Update tests.

@desilinguist
desilinguist requested review from a user, Lguyogiro, aoifecahill, farigys and mulhod June 5, 2020 13:59
@codecov

codecov Bot commented Jun 5, 2020 •

Copy link
Copy Markdown

Codecov Report

Merging #619 into master will increase coverage by 3.43%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   91.71%   95.15%   +3.43%     
==========================================
  Files          26       26              
  Lines        3031     3031              
==========================================
+ Hits         2780     2884     +104     
+ Misses        251      147     -104     
Impacted Files Coverage Δ
skll/config/__init__.py 95.09% <ø> (ø)
skll/utils/constants.py 100.00% <ø> (ø)
skll/learner/__init__.py 96.11% <100.00%> (+6.37%) ⬆️
skll/metrics.py 97.87% <0.00%> (+2.12%) ⬆️
skll/experiments/__init__.py 95.19% <0.00%> (+6.60%) ⬆️
skll/experiments/output.py 97.36% <0.00%> (+6.84%) ⬆️
skll/config/utils.py 95.65% <0.00%> (+8.69%) ⬆️
skll/learner/utils.py 95.88% <0.00%> (+12.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf31ab...5d2b906. Read the comment docs.

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks good. Can we also have a test case with empty dictionary parameter grid.

@desilinguist

Copy link
Copy Markdown
Collaborator Author

Almost all of our tests in test_classification.py and test_regression.py are with empty parameter grids since we do not specify an explicit parameter grid either via the API or via the config file. So that's already covered.

@desilinguist
desilinguist merged commit 7e413e8 into master Jun 5, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the 618-simplify-default-param-grids branch June 5, 2020 16:40
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.

Simplify parameter grids to be dictionaries instead of lists of dictionaries

3 participants