master: Fixed hyperparameter collisions #806
Conversation
|
model JiGen is still missing? |
|
Trainers like mldg dial also have gamma, that can be done also with id? |
|
From what I can see, jigen has not hyper_init and hyper_update functions, so I assumed hyperparameters are not updated. To the other question: In your initial issue, you split it into two parts. The first part, the gamma collision when combining trainers, is not addressed with this. The second part, where the naming of model parameters collides, is. Will need to think about the first part, as this won't work with ids |
you are right, i forgt, sorry, jigen is a subclass of dann |
|
I read the pr several times since it first launched, the key point i want to make sure is that this design will be consistent with extensions, for example, if we have hyperscheduler_dial_mldg trainer, training diva_hduva model. |
…s different gamma values for different trainers and models based on the naming of the class. Also added parameter printing
MatteoWohlrapp
changed the title
|
Solved gamma_reg naming collision by introducing functionality to pass different gamma values for different trainers and models based on the naming of the class. With this comes a slight change in how the command line or yaml arguments are passed, however, the previous format is still supported. Also added parameter printing for models and trainers. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #806 +/- ##
==========================================
+ Coverage 95.17% 95.23% +0.05%
==========================================
Files 128 129 +1
Lines 5080 5138 +58
==========================================
+ Hits 4835 4893 +58
Misses 245 245
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@MatteoWohlrapp , could you fix the yaml according to my review? |
|
Can't see a review |
|
|
|
@MatteoWohlrapp, the test coverage must be above 95% currently Attention: Patch coverage is 88.05970% with 8 lines in your changes are missing coverage. Please review. |
@MatteoWohlrapp see detailed coverage report in quotes. |
…ser and specified input types
smilesun
changed the title
docs/doc_usage_cmd.md
Outdated
|
|
||
| - **Command Line Usage:** | ||
| - For a single value: `python script.py --gamma_reg=0.1` | ||
| - For multiple values: `python script.py --gamma_reg='default=0.1,dann=0.05,diva=0.2'` |
There was a problem hiding this comment.
actually, due to the specific implementation of DIVA, instead of
There was a problem hiding this comment.
But where is this specified for model diva. I can only find this initialization of diva which does not have a regularization :
model = mk_diva(list_str_y=task.list_str_y)(
node,
zd_dim=args.zd_dim,
zy_dim=args.zy_dim,
zx_dim=args.zx_dim,
list_d_tr=task.list_domain_tr,
gamma_d=args.gamma_d,
gamma_y=args.gamma_y,
beta_x=args.beta_x,
beta_y=args.beta_y,
beta_d=args.beta_d,
)
It's here
There was a problem hiding this comment.
| gamma_reg: 1.0 # hyperparameter of DANN | ||
| gamma_reg: | ||
| default: 1.0 | ||
| dann: 1.5 |
There was a problem hiding this comment.
@MatteoWohlrapp , how about mldg?
There was a problem hiding this comment.
Will get the default values assigned
There was a problem hiding this comment.
is this behavior tested somewhere? in which test, for instance, now the gamma_reg value for mldg should be 1.0? @MatteoWohlrapp
There was a problem hiding this comment.
i made a pull request to test his, still under work @MatteoWohlrapp
|
lookes like error introduced by my commit |
you corrected that already in the last two commits. |
Introduced a unique id for every object, which is added to the parameter name.