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.

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

Feature/improve loss functions #70

Open
wants to merge 54 commits into
base: develop
Choose a base branch
from
Open

Feature/improve loss functions #70

wants to merge 54 commits into from

Conversation

HCookie
Copy link
Member

@HCookie HCookie commented Oct 2, 2024 •
edited by github-actions bot
Loading

Improve loss functions.

Prior, WeightedMSELoss was the hard coded loss for training and metrics.
This PR changes the loss function configuration to be fully modular, so that any nn.Module can be used.

Additionally, this adds the following additonal loss functions into anemoi-training

  • WeightedMAE
  • WeightedRMSE
  • WeightedLogCosh
  • WeightedHuber
  • CombinedLoss

Arbitary Scalars

See #96 for more information

  • Allows for arbitary scalars to be applied to the loss values

📚 Documentation preview 📚: https://anemoi-training--70.org.readthedocs.build/en/70/

@HCookie HCookie self-assigned this Oct 2, 2024
@HCookie HCookie added the enhancement New feature or request label Oct 2, 2024
@HCookie HCookie marked this pull request as ready for review October 2, 2024 15:59
JesperDramsch
JesperDramsch requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@JesperDramsch JesperDramsch left a comment

Choose a reason for hiding this comment

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

Thanks for tackling these.

I have some doubts about some design decisions, as I have also mentioned in a conversation.

Could you please elaborate why the loss functions now have a dynamic feature index?

I think the metrics could be a nice list, so we can calculate multiple metrics as well.

src/anemoi/training/losses/mse.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/mse.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/mse.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/mse.py Outdated Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
src/anemoi/training/config/training/default.yaml Outdated Show resolved Hide resolved
src/anemoi/training/config/training/default.yaml Outdated Show resolved Hide resolved
src/anemoi/training/losses/mse.py Show resolved Hide resolved
src/anemoi/training/losses/mse.py Outdated Show resolved Hide resolved
src/anemoi/training/config/training/default.yaml Outdated Show resolved Hide resolved
@HCookie
Address PR Comments
91e3ae3 
- Add comment to if dim == 4
- Move to inplace out multiplication
@HCookie
Allow for lists of losses
23c1670
@HCookie
Fix ruff complaining
78d6588
@HCookie HCookie mentioned this pull request Oct 3, 2024
Draft
@HCookie
refactor: Create WeightedLoss
5f25370 
- Is an abstract weighted loss class
- Combines feature weighting and node weighting
- mse, mae, rmse, logcosh now subclass from WeightedLoss
@HCookie
fix: Assert and conversion in loss function get
1054f7f
@HCookie
Fix names to reference weighted
19c1799
@HCookie
fix: rework init include loop to raise an error if missing key found
ef49cbb
@HCookie
fix: refine documentation for feature_weights
aa605a6
@HCookie
Fix: Remove reduction over ensemble
7c4c8cb 
- Was causing issues with training, due to new shape of data
JesperDramsch
JesperDramsch previously requested changes Oct 8, 2024
View reviewed changes
Copy link
Member

@JesperDramsch JesperDramsch left a comment

Choose a reason for hiding this comment

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

I left a partial review on some of the core problems I see with the current implementation. Thank you for already addressing a lot of my previous comments.

I think there are a few main issues from my side regarding the initialization of the losses. It's likely we could just solve this in a quick call rather than asynchronously.

src/anemoi/training/losses/weightedloss.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/weightedloss.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/weightedloss.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/weightedloss.py Outdated Show resolved Hide resolved
src/anemoi/training/losses/weightedloss.py Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
src/anemoi/training/train/forecaster.py Outdated Show resolved Hide resolved
@HCookie
Rename WeightedLoss to BaseWeightedLoss
eb7bded 
- Also chage latitude to Node
@HCookie
Copy link
Member Author

HCookie commented Oct 18, 2024 •
edited
Loading

Provides a way to use arbitary scalings
#96

@mc4117
Copy link
Member

mc4117 commented Oct 21, 2024

We now have a downstream application which depends on this branch so it would be nice if it could be merged soonish

HCookie and others added 7 commits October 22, 2024 14:15
@HCookie
Add Huber Loss
df1c92a
@HCookie
Add CombinedLoss
6dcc9b7 
- Allows for multiple losses to be combined with associated weights
@HCookie
Merge branch 'develop' into feature/improve_loss_functions
202011f
@HCookie
Add getattr to CombinedLoss
ec2c204 
- Allows access to underlying functions
@HCookie
Add ScaleTensor (#96)
da89d99 
* Add ScaleTensor
- Allows dynamic setup of scalars
- Rework variable_scaler to use it
@HCookie
Update copyright notice
8392873
@HCookie
Merge branch 'develop' into feature/improve_loss_functions
a628fae
@HCookie HCookie dismissed JesperDramsch’s stale review October 23, 2024 15:56

JesperDramsch is currently absent

HCookie and others added 9 commits October 24, 2024 08:02
@HCookie
Upadate copyright notice
11fe213
@HCookie
Improve config documentation
7694733
@HCookie
Fix initalisation bugs
5c7ed01 
- Also fix scalar being incorrectly made
@HCookie
Add tests to ensure correct initalisation
07a2a8b
@HCookie
Add docs to add_scalar
fda2085 
- Info about indexing
@HCookie
Merge branch 'develop' into feature/improve_loss_functions
1892ad6
@HCookie
Refactor of loss functions
8a17fb1 
- Remove 'include_', and 'add_scalar_'
- Add 'scalars' to control scalars
- Add docs
- Rework tests
- Add functional loss function
- Remove variable_scaling from kwargs
- Require subclass of BaseWeightedLoss
@HCookie
Update docs
2e9f721
@HCookie
Reorder docs
22cd154
@HCookie HCookie requested a review from mchantry October 25, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floriankrb floriankrb floriankrb left review comments

@mc4117 mc4117 mc4117 left review comments

@sahahner sahahner sahahner left review comments

@JesperDramsch JesperDramsch JesperDramsch left review comments

@gmertes gmertes Awaiting requested review from gmertes

@mishooax mishooax Awaiting requested review from mishooax

@gabrieloks gabrieloks Awaiting requested review from gabrieloks

@ssmmnn11 ssmmnn11 Awaiting requested review from ssmmnn11

@Rilwan-Adewoyin Rilwan-Adewoyin Awaiting requested review from Rilwan-Adewoyin

@mchantry mchantry Awaiting requested review from mchantry

At least 1 approving review is required to merge this pull request.

Assignees

@HCookie HCookie

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@HCookie @mc4117 @JesperDramsch @floriankrb @mchantry @Rilwan-Adewoyin @sahahner