E52A Cleanup config files by sfluegel05 · Pull Request #144 · ChEB-AI/python-chebai · GitHub
[go: up one dir, main page]
Skip to content

Cleanup config files #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
sfluegel05 merged 9 commits into from
Jan 28, 2026
Merged

Cleanup config files #144

sfluegel05 merged 9 commits into from
Jan 28, 2026

Conversation

@sfluegel05
Copy link
Collaborator
@sfluegel05 sfluegel05 commented Jan 22, 2026 •
edited
Loading

We have a lot of config files that do different things which might be confusing to new users and lead to unexpected results. For example, the callback configs sometimes save a checkpoint every 25 epochs, sometimes they only save the last checkpoint. Sometimes, they save the best 3, sometimes only the best 1.

I tried to reduce the number of configs and find a consistent naming pattern. More precisely, I

  • removed the weightings directory and loss/weighting_chebi100.yml - those were all static lists of weights which are not needed anymore since those weights get calculated on the fly by the weighted BCE.
  • removed the metrics which had a specific label number attached. Those label numbers are automatically inferred from the dataset
  • restored the default trainer and default callbacks to their original state (which means removing the roc-auc). This fixes Missing labels in loss kwargs causing issue for electra model / Issues from PR#130 #140 (comment). For other callbacks, the roc-auc is still used (e.g. binary_callbacks.yml and early_stop_callbacks_roc-auc.yml
  • renamed the solCur and tox21 callbacks (as far as I can tell, they are not specific to the dataset)
  • removed some comments (I assume that those parts are outdated)
  • added an upper limit of 1 to the weighted BCE's beta parameter

solves #139

@sfluegel05 sfluegel05 requested a review from schnamo January 22, 2026 12:38
@sfluegel05
Copy link
Collaborator Author
sfluegel05 commented Jan 22, 2026

@schnamo Could you please have a look if this makes sense to you or if I have removed anything important?

@sfluegel05
Copy link
Collaborator Author
sfluegel05 commented Jan 22, 2026

One more fix: The current_epoch parameter only works if a loss function expects it (or at least ignores unknown kwargs). This is not the case for the unweighted BCE loss. Fortunately, we can use the pass_loss_kwargs flag to avoid this issue.

@aditya0by0
Copy link
Member
aditya0by0 commented Jan 26, 2026

python-chebai/configs/loss/bce_unweighted.yml

Line 1 in 395b5a8

class_path: torch.nn.BCEWithLogitsLoss

I think the raw torch module for BCE loss will throw an error, as we pass some kwargs to forward method which is not accepted by it.

Instead the config should use the wrapper from

python-chebai/chebai/loss/bce_weighted.py

Lines 108 to 111 in 395b5a8

class UnWeightedBCEWithLogitsLoss(torch.nn.BCEWithLogitsLoss):
def forward(self, input, target, **kwargs):
# As the custom passed kwargs are not used in BCEWithLogitsLoss, we can ignore them
return super().forward(input, target)

@schnamo
Copy link
Collaborator
schnamo commented Jan 26, 2026

Looks great, thank you Simon!

schnamo
schnamo approved these changes Jan 26, 2026
View reviewed changes
Copy link
Collaborator
@schnamo schnamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good, thank you

aditya0by0
aditya0by0 reviewed Jan 26, 2026
View reviewed changes
@aditya0by0
Copy link
Member
aditya0by0 commented Jan 26, 2026

Do we need to retain certain comments from PR #130? ... like below

python-chebai/chebai/cli.py

Lines 87 to 96 in 395b5a8

# parser.link_arguments(
# "model.init_args.out_dim", "trainer.callbacks.init_args.num_labels"
# )
# parser.link_arguments(
# "data", "model.init_args.criterion.init_args.data_extractor"
# )
# parser.link_arguments(
# "data.init_args.chebi_version",
# "model.init_args.criterion.init_args.data_extractor.init_args.chebi_version",
# )

python-chebai/chebai/cli.py

Lines 73 to 77 in 395b5a8

# When using lightning > 2.5.1 then need to uncomment all metrics that are not used
# for average in ("mse", "rmse","r2"): # for regression
# for average in ("f1", "roc-auc"): # for binary classification
# for average in ("micro-f1", "macro-f1", "roc-auc"): # for multilabel classification
# for average in ("micro-f1", "macro-f1", "balanced-accuracy", "roc-auc"): # for multilabel classification using balanced-accuracy

...
...
etc

@sfluegel05
Copy link
Collaborator Author
sfluegel05 commented Jan 28, 2026

Do we need to retain certain comments from PR #130? ... like below

I don't think we need those comments.

As discussed in our last meeting, I added the custom unweighted BCE class to the config.

@sfluegel05 sfluegel05 linked an issue Jan 28, 2026 that may be closed by this pull request
Sort loss configs #139
Closed
@sfluegel05 sfluegel05 merged commit 1020753 into dev Jan 28, 2026
5 checks passed
@sfluegel05 sfluegel05 deleted the fix/checkpoint-metrics branch January 28, 2026 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aditya0by0 aditya0by0 aditya0by0 left review comments

@schnamo schnamo schnamo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing labels in loss kwargs causing issue for electra model / Issues from PR#130 Sort loss configs

4 participants

@sfluegel05 @aditya0by0 @schnamo
0