-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pipeline_space as yaml input #46
Conversation
…y pre-commit in search space
…ew tests for optional type argument
…ils for yaml search space in own file
…e notation usage for all parameters and arguments
The directories were restructured. Needs to be updated with the |
Done. |
…or paramter validation functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature addition looks good. I've tested the example, and it worked as expected.
The inclusion of testing is a great addition, adding robustness to the codebase.
assert pipeline_space["learning_rate"].lower == 0.00001 | ||
assert pipeline_space["learning_rate"].upper == 0.1 | ||
assert pipeline_space["learning_rate"].log is True | ||
assert pipeline_space["learning_rate"].is_fidelity is False | ||
assert pipeline_space["learning_rate"].default == 3.3e-2 | ||
assert pipeline_space["learning_rate"].default_confidence_score == 0.125 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danrgll @TarekAbouChakra
For this PR let us push this.
For later refactoring, can we compress such lines by creating a FloatParameter
using these values explicitly in one line, and comparing the loaded instantiation using something like this?
If so, let us keep a note and do this for next year's release.
def test_correct_including_priors_yaml_file(): | ||
"""Test the function with a correctly formatted YAML file.""" | ||
pipeline_space = pipeline_space_from_yaml( | ||
"tests/test_yaml_search_space/correct_config_including_priors.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we perhaps make the location (tests/test_yaml_search_space/
) a global or class variable for all such relative path imports?
(Same, please note for the next release)
with pytest.raises(SearchSpaceFromYamlFileError) as excinfo: | ||
pipeline_space_from_yaml( | ||
Path("tests/test_yaml_search_space/inconsistent_types_config2.yml") | ||
) | ||
assert str(excinfo.value.exception_type == "TypeError") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a boolean expression is converted to either "True" or "False" the assertion
will always be True.
Are we sure these test cases pass?
We should check these before releasing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. im going to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixedhere
Looks good! There is only one issue with the tests cases for the wrong YAML inputs where I think the test is not being checked correctly. Overall, it looks quite comprehensive with what looks like adequate documentation! One more such point is, can we now generate a yaml from a |
d11e4c7
to
075c79b
Compare
@TarekAbouChakra @danrgll |
Enabled initialization of search_space via a YAML file.
You can find an example in neps_examples/basic_usage/defining_search_space. For more information, refer to docs/pipeline_space.md.
Supported Parameter Types: Int, Float, Categorical, Constant