Skip to content

feat(costs): Add dunder methods to costs#243

Merged
nicola-bastianello merged 4 commits intoteam-decent:mainfrom
adrianardv:feat/178-cost-composition
Feb 8, 2026
Merged

feat(costs): Add dunder methods to costs#243
nicola-bastianello merged 4 commits intoteam-decent:mainfrom
adrianardv:feat/178-cost-composition

Conversation

@adrianardv
Copy link
Contributor

@adrianardv adrianardv commented Feb 5, 2026

Summary

  • Add scalar multiplication, division, negation, subtraction, and reverse add for Cost
  • Introduce ScaledCost to represent weighted objectives
  • Add operator tests for the new behaviours

Closes #178

Adriana Rodriguez added 3 commits February 5, 2026 18:14
Add scalar multiplication/division, negation, subtraction, and reverse
addition for costs. Introduce ScaledCost for weighted objectives and
cover the behavior with operator tests.

closes team-decent#178
self.cost = cost.cost
self.scalar = scalar * cost.scalar
else:
self.cost = cost
Copy link
Member

Choose a reason for hiding this comment

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

the way we assign self.cost here is as a reference to cost, since that's the default in python. I'm thinking that maybe we should do a deepcopy to avoid unexpected behavior

@Simpag would there be any problem making deep copies of pytorch costs?

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be an issue, we'd need to investigate this. Currently the SumCost does not copy the cost functions either. It should be fine to deep copy but it might cause massive memory usage if big models or datasets are used. For empirical costs the issue might be that the batch_used property gets updated if you use the SumCost/ScaledCost.

But I am not sure, this should be tested.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I hadn't realized self._n_function_calls was also doing this (there's always something that goes unnoticed).

then I'm inclined to merge the PR with this implementation, and after we test both SumCost and ScaledCost we can come back to the discussion. I opened #244

@nicola-bastianello
Copy link
Member

a couple of other things:

  • maybe we can add the supported operations in the user guide (just a list is fine, no need to show the code)
  • @Simpag do we need to use @iop.autodecorate_cost_method for the methods of ScaledCost?

@nicola-bastianello
Copy link
Member

another thing: I think maybe we should not translate scalar/cost as cost/scalar. it could lead to very unexpected behavior

@Simpag
Copy link
Contributor

Simpag commented Feb 6, 2026

a couple of other things:

  • maybe we can add the supported operations in the user guide (just a list is fine, no need to show the code)
  • @Simpag do we need to use @iop.autodecorate_cost_method for the methods of ScaledCost?

The @iop.autodecorate_cost_method is not needed if you use the base class like this. It is only useful when you want to convert the input arguments to something that's not Array, which is taken care of in the actual cost function implementations. One could create cost functions which are completely framework agnostic by only using the iop functions but it is more convenient to just use the framework you want to create the cost function for within the cost methods (can also improve efficiency, like the PyTorchCost).

@Simpag
Copy link
Contributor

Simpag commented Feb 6, 2026

another thing: I think maybe we should not translate scalar/cost as cost/scalar. it could lead to very unexpected behavior

It should be possible to have "scalar/cost", could have an init parameter in the scaled cost to inverse the output of the cost function to (1/method_value) in all calls or create something like "InverseCost" which is just "1/cost".

However, I’m not sure how useful this would be or if it would ever be used. It would cause issues when we have division by zero. I would imagine that this could pop up somewhat frequently, especially for gradients. Maybe we just should not support it.

@nicola-bastianello
Copy link
Member

another thing: I think maybe we should not translate scalar/cost as cost/scalar. it could lead to very unexpected behavior

It should be possible to have "scalar/cost", could have an init parameter in the scaled cost to inverse the output of the cost function to (1/method_value) in all calls or create something like "InverseCost" which is just "1/cost".

However, I’m not sure how useful this would be or if it would ever be used. It would cause issues when we have division by zero. I would imagine that this could pop up somewhat frequently, especially for gradients. Maybe we just should not support it.

yes cannot think of any situation where this would be useful honestly. I would avoid supporting it altogether

@adrianardv
Copy link
Contributor Author

Yes, I also can’t think of a practical use for scalar / cost, and it could easily run into issues. I originally mapped it to cost / scalar for convenience, but I agree that’s misleading. I’ve now changed __rtruediv__ to raise a TypeError so scalar / cost is explicitly unsupported.

Reject scalar / cost, validate proximal parameters for scaled costs, and
document supported cost operations in the user guide. Update tests and
formatting accordingly.
@nicola-bastianello nicola-bastianello merged commit 9e677ed into team-decent:main Feb 8, 2026
7 checks passed
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.

Enhancing cost_functions

3 participants