add_set_params_and_corresponding_tests #1102
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #1102 +/- ##
==========================================
+ Coverage 86.99% 87.13% +0.13%
==========================================
Files 170 170
Lines 9326 9340 +14
==========================================
+ Hits 8113 8138 +25
+ Misses 1213 1202 -11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I request a review |
There was a problem hiding this comment.
As I can see from the #1025, it is expected that we create a new object, not change the current.
|
In case you worry that I left the project silently, I'm still working on the issue and will submit code tomorrow. |
|
Changed implementation to creating new object instead of modifying self |
… into base_mixin_set_params Merge changes from main
etna/core/mixins.py
Outdated
| """ | ||
| params_dict = self.to_dict() | ||
|
|
||
| # we need to change specification into list of nested params, "model.depth" to nested structure |
There was a problem hiding this comment.
This description is not very easy to understand.
Let's write something like:
We need to change specification into list of nested params, e.g. {"model.depth": value} to {"model": {"depth": value}}.
I think that we should extract this logic into private method for better readability. For example, _convert_dict_flat_to_nested.
There was a problem hiding this comment.
May be we should also write a couple of tests for this conversion.
There was a problem hiding this comment.
I agree, I'll change the specification and write tests. It is generally a good idea to isolate code performing interpretable side tasks into distinct functions.
There was a problem hiding this comment.
I'm going to create _update_nested_dict_with_flat_dict(self, params_dict: dict, flat_dict: dict) -> dict method.
I think the problem with _convert_dict_flat_to_nested is that once we have two nested dicts, standart python dict merge overrides values with same keys. As example
dict_1 = {"model" : {"learning_rate": 3e-4}}
dict_2 = {"model" : {"depth" : 8}}
dict_1.update(dict_2)
will result in
dict_1 = {"model" : {"depth" : 8}}
instead of desired {"learning_rate": 3e-4, "model" : {"depth" : 8}}.
So I believe the private method should perform two operations simultaneously: convert from "model.param" : val to "model" : {"param" : val} and immediately update params_dict.
Are you okay with this?
There was a problem hiding this comment.
Probably, it can be solved by:
- obj -> nested dict
- nested dict -> flat dict
- update flat dict
- flat dict -> nested dict
- creating object from nested dict
But we don't need this complexity right now. Probably we will made something like this in the future.
Before submitting (must do checklist)
Proposed Changes
See issue #1025
Closing issues
closes #1025