megvii-research / hpman

A hyperparameter manager for deep learning experiments.
MIT License
95 stars 11 forks source link

Implement nested hyper params by flattening keys #4

Closed bigeagle closed 2 years ago

bigeagle commented 5 years ago

@zxytim I tested the code

from hpman.m import _
_('a.b', 1)
_('a', {'c': 1})
_('a.c')

Here _('a.c') would return an EmptyValue and get_tree would just fail with "Impossible tree"

bigeagle commented 5 years ago

TODO:

from hpman.m import _
_('a.b', 1)        # {'a': {'b': 1}}
_('a', {'c': 1})   # fail static parsing
from hpman.m import _
_('a', {'c': 1})   # {'a': {'c', 1}}
_('a.c')            # fail static parsing, runtime raises KeyError
from hpman.m import _
_('a', {'c': 1})   # {'a': {'c', 1}}
_.set_values('a.c', 2)  # {'a': {'c': 2}} override static result
bigeagle commented 5 years ago

@zxytim I came up with another case that tree and dict are incompatible.

# SomeKey is not a string
_('a', {SomeKey: val})
bigeagle commented 5 years ago

@zxytim pls review and give harsh test to this commit.

zxytim commented 5 years ago

@bigeagle Should we support subclasses of str as dict key? A simple test would fail by now:

from hpman.m import _

class MyStr(str):
    pass

b = MyStr("b")

_("a", {b: 1})
print(_.get_value("a"))
print(_.get_value("a.b"))
$ python3 t.py
{'b': 1}
Traceback (most recent call last):
  File "t.py", line 12, in <module>
    print(_.get_value("a.b"))
  File "/home/zxytim/project/hpman/hpman/hpm.py", line 293, in get_value
    raise KeyError("`{}` not found".format(name))
KeyError: '`a.b` not found'

I would like to know your suggestions.

bigeagle commented 5 years ago

The code you provided is not a good example.

from hpman.m import _

_("a", {"b": 1})
print(_.get_value("a"))
print(_.get_value("a.b"))

This will also fail with KeyError. Nested key syntax for dict indexing is not supported due to type-safety.

zxytim commented 5 years ago

I see.

I also found tests regarding HyperParamTree.DICT_ANNOTATION are missing, would you please add some tests?

bigeagle commented 5 years ago

I see.

I also found tests regarding HyperParamTree.DICT_ANNOTATION are missing, would you please add some tests?

They are here and here.

bigeagle commented 5 years ago

TODO:

Add documentation.

sshao0516 commented 5 years ago

Codecov Report

Merging #4 into master will increase coverage by 0.22%. The diff coverage is 99.13%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4      +/-   ##
=======================================
+ Coverage   98.77%   99%   +0.22%     
=======================================
  Files           7     7              
  Lines         327   402      +75     
=======================================
+ Hits          323   398      +75     
  Misses          4     4
Impacted Files Coverage Δ
hpman/__init__.py 100% <ø> (ø) :arrow_up:
hpman/primitives.py 100% <100%> (ø) :arrow_up:
hpman/hpm.py 99.24% <98.43%> (-0.76%) :arrow_down:
hpman/hpm_db.py 99.43% <99.37%> (+0.75%) :arrow_up:

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 72fb244...44217d5. Read the comment docs.