Skip to content
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.

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

Hybrid ce loss changes #364

Merged
merged 67 commits into from
Jun 23, 2023
Merged

Hybrid ce loss changes #364

merged 67 commits into from
Jun 23, 2023

Conversation

zhiyil1230
Copy link
Contributor

Changelogs

  • enumerate the changes of that PR.

Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the new introduced feature(s) (if appropriate).
  • Update the API documentation is a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix or test (or ask a maintainer to do it for you).

discussion related to that PR

Fix to hybridceloss, but it is not actually helping the hybridceloss for val and test using the debug config.

hadim and others added 30 commits June 7, 2023 07:05
@zhiyil1230 zhiyil1230 changed the title Hybrid c eloss changes Hybrid ce loss changes Jun 22, 2023
@zhiyil1230 zhiyil1230 changed the base branch from merging_configs to main June 23, 2023 14:01
regression_input = torch.inner(input, self.brackets.to(input.device))
# regression loss needs normalized logits to probability as input to do inner product with self.brackets
# we apply softmax on the raw logits first
softmax_input = self.softmax(input)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we discussed that the softmax should go as the last_activation since we want the classification metrics to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned about this on slack about the auroc and average precision metrics. It looks like they are happy with both probabilities or logits. In that case we don't need a custom CE loss? Or do you think there would be other classification metrics which only accept probabilities as the input?

`    Accepts the following input tensors:

    - ``preds`` (float tensor): ``(N, C, ...)``. Preds should be a tensor containing probabilities or logits for each
      observation. If preds has values outside [0,1] range we consider the input to be logits and will auto apply
      softmax per sample.`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, just saw your Slack message!

I checked also Accuracy and f1. I think this is consistent across metrics 🎉

@DomInvivo DomInvivo merged commit 892de3b into main Jun 23, 2023
@DomInvivo DomInvivo deleted the hybridCEloss_changes branch July 21, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants