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

Refactor the code related to loss computation #965

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

szmazurek
Copy link
Collaborator

Fixes #854

Proposed Changes

  • create a common interface for the loss functions
  • refactor the codebase to use new loss implementations

Checklist

  • CONTRIBUTING guide has been followed.
  • PR is based on the current GaNDLF master .
  • Non-breaking change (does not break existing functionality): provide as many details as possible for any breaking change.
  • Function/class source code documentation added/updated (ensure typing is used to provide type hints, including and not limited to using Optional if a variable has a pre-defined value).
  • Code has been blacked for style consistency and linting.
  • If applicable, version information has been updated in GANDLF/version.py.
  • If adding a git submodule, add to list of exceptions for black styling in pyproject.toml file.
  • Usage documentation has been updated, if appropriate.
  • Tests added or modified to cover the changes; if coverage is reduced, please give explanation.
  • If customized dependency installation is required (i.e., a separate pip install step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].
  • The logging library is being used and no print statements are left.

@szmazurek szmazurek requested a review from a team as a code owner November 9, 2024 20:21
Copy link
Contributor

github-actions bot commented Nov 9, 2024

MLCommons CLA bot:
Thank you very much for your submission, we really appreciate it. Before we can accept your contribution, we ask that you sign the MLCommons CLA (Apache 2). Please use this [Google form] (https://forms.gle/Ew1KkBVpyeJDuRw67) to initiate authorization. If you are from an MLCommons member organization, we will request that you be added to the CLA. If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact [email protected].
0 out of 1 committers have signed the MLCommons CLA.
@szymon Mazurek
Szymon Mazurek seems not to be a GitHub user. You need a GitHub account after you become MLCommons member. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

@szmazurek szmazurek marked this pull request as draft November 9, 2024 20:22
@szmazurek
Copy link
Collaborator Author

@sarthakpati What is your opinion on the implementation of interface and example loss? When we agree on that I will proceed to port the rest.

@sarthakpati
Copy link
Collaborator

Should this PR not be going into https://github.com/szmazurek/GaNDLF/tree/dev ?

@szmazurek
Copy link
Collaborator Author

Should this PR not be going into https://github.com/szmazurek/GaNDLF/tree/dev ?

Could be, but as it is directly related to existing core code I tought that we can update directly here.

@sarthakpati sarthakpati changed the title Refactor loss Refactor the code related to loss computation Nov 13, 2024
@sarthakpati sarthakpati marked this pull request as ready for review November 13, 2024 15:26
@sarthakpati
Copy link
Collaborator

This commit was done without proper git initialization and hence is getting flagged by the CLA bot. Can you rebase this?

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.

[BUG] Loss signatures: CE Loss failure because of additional params argument
2 participants