master: Fixed hyperparameter collisions

MatteoWohlrapp commented 2 months ago

Introduced a unique id for every object, which is added to the parameter name.

smilesun commented 2 months ago

model JiGen is still missing?

smilesun commented 2 months ago

Trainers like mldg dial also have gamma, that can be done also with id?

MatteoWohlrapp commented 2 months ago

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

smilesun commented 2 months ago

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

smilesun commented 2 months ago

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.

MatteoWohlrapp commented 2 months ago

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-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.23%. Comparing base (faf7589) to head (0d62919).

:exclamation: Current head 0d62919 differs from pull request most recent head bdf84c2

Please upload reports for the commit bdf84c2 to get more accurate results.

Additional details and impacted files

```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/marrlab/DomainLab/pull/806/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/marrlab/DomainLab/pull/806/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab) | `95.23% <100.00%> (+0.05%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=marrlab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

smilesun commented 2 months ago

@MatteoWohlrapp , could you fix the yaml according to my review?

MatteoWohlrapp commented 2 months ago

Can't see a review

smilesun commented 2 months ago

see the screenshot, the same scene not on your side?

smilesun commented 2 months ago

@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.

smilesun commented 2 months ago

Codecov Report

Attention: Patch coverage is 88.05970% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 95.00%. Comparing base (af8a238) to head (cc650ad). Report is 4 commits behind head on master.

Files Patch % Lines domainlab/models/a_model.py 78.57% 3 Missing ⚠️ domainlab/algos/trainers/a_trainer.py 33.33% 2 Missing ⚠️ domainlab/arg_parser.py 83.33% 2 Missing ⚠️ domainlab/utils/hyperparameter_retrieval.py 88.88% 1 Missing ⚠️ Additional details and impacted files ☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.

@MatteoWohlrapp see detailed coverage report in quotes.

smilesun commented 2 months ago

@MatteoWohlrapp , what is the test coverage at the moment?

One way to find this is run the pytest locally on a GPU node, or even better, change the CI yaml to display the coverage each time

MatteoWohlrapp commented 2 months ago

You can also check it on the Codacity website here or refer to the screenshot below; it seems like it is fine now:

smilesun commented 1 month ago

@MatteoWohlrapp

smilesun commented 1 week ago

@MatteoWohlrapp

Run poetry run python examples/api/jigen_dann_transformer.py
Traceback (most recent call last):
  File "/home/runner/work/DomainLab/DomainLab/examples/api/jigen_dann_transformer.py", line 9, in <module>
    from domainlab.mk_exp import mk_exp
  File "/home/runner/work/DomainLab/DomainLab/domainlab/mk_exp.py", line 4, in <module>
    from domainlab.arg_parser import mk_parser_main
  File "/home/runner/work/DomainLab/DomainLab/domainlab/arg_parser.py", line 33
    my_dict = {}        if "=" in values:
                                        ^
SyntaxError: invalid syntax
Error: Process completed with exit code 1.

smilesun commented 1 week ago

@MatteoWohlrapp

Run poetry run python examples/api/jigen_dann_transformer.py
Traceback (most recent call last):
  File "/home/runner/work/DomainLab/DomainLab/examples/api/jigen_dann_transformer.py", line 9, in <module>
    from domainlab.mk_exp import mk_exp
  File "/home/runner/work/DomainLab/DomainLab/domainlab/mk_exp.py", line 4, in <module>
    from domainlab.arg_parser import mk_parser_main
  File "/home/runner/work/DomainLab/DomainLab/domainlab/arg_parser.py", line 33
    my_dict = {}        if "=" in values:
                                        ^
SyntaxError: invalid syntax
Error: Process completed with exit code 1.

lookes like error introduced by my commit

smilesun commented 1 week ago

@MatteoWohlrapp

Run poetry run python examples/api/jigen_dann_transformer.py
Traceback (most recent call last):
  File "/home/runner/work/DomainLab/DomainLab/examples/api/jigen_dann_transformer.py", line 9, in <module>
    from domainlab.mk_exp import mk_exp
  File "/home/runner/work/DomainLab/DomainLab/domainlab/mk_exp.py", line 4, in <module>
    from domainlab.arg_parser import mk_parser_main
  File "/home/runner/work/DomainLab/DomainLab/domainlab/arg_parser.py", line 33
    my_dict = {}        if "=" in values:
                                        ^
SyntaxError: invalid syntax
Error: Process completed with exit code 1.

lookes like error introduced by my commit

you corrected that already in the last two commits.

marrlab / DomainLab

master: Fixed hyperparameter collisions #806

Codecov Report

Codecov Report