Aarhus-Psychiatry-Research / psycop-model-training

Shared code for model training and evaluation.
Other
1 stars 0 forks source link

Offset negative values #472

Closed signekb closed 1 year ago

signekb commented 1 year ago

Notes for reviewers

Adding the option to offset column values by most negative value, so all values are non-negative.

github-actions[bot] commented 1 year ago

Test Results

22 tests   22 :heavy_check_mark:  16s :stopwatch:   1 suites    0 :zzz:   1 files      0 :x:

Results for commit 9ea2a58a.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 1 year ago
Tests Skipped Failures Errors Time
23 0 :zzz: 0 :x: 0 :fire: 2m 38s :stopwatch:
signekb commented 1 year ago

It fails bc I have added "self.pre_split_cfg.offset_negative_values" which is not in the cfg. I remember that you've said that we should try to avoid adding variables to cfgs (bc everyone will need to add it to their projects), but unsure how else it can be implemented?
Since we are now running with f_classif instead of chi2, this is not a big issue for us anymore, but might be nice to have as an option 👍

MartinBernstorff commented 1 year ago

It fails bc I have added "self.pre_split_cfg.offset_negative_values" which is not in the cfg. I remember that you've said that we should try to avoid adding variables to cfgs (bc everyone will need to add it to their projects), but unsure how else it can be implemented? Since we are now running with f_classif instead of chi2, this is not a big issue for us anymore, but might be nice to have as an option 👍

Yeah, totally!

When running tests, we use the config objects from the tests directory. To fix the error, you just need to add the key to those files :-)

github-actions[bot] commented 1 year ago

This PR is stale because it has been open 1+ days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

github-actions[bot] commented 1 year ago

This PR is stale because it has been open 1+ days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

github-actions[bot] commented 1 year ago

✨ Looks like mypy failed ✨

If you want to fix this, we recommend doing it locally by:

  1. Installing mypy, which is included in the dev dependencies: pip install -e ".[dev]"
  2. Diagnosing the errors by running mypy .
github-actions[bot] commented 1 year ago

This PR is stale because it has been open 1+ days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

signekb commented 1 year ago

Changed the things you requested 👍

MartinBernstorff commented 1 year ago

Looks awesome, merging!