Skip to content

Conversation

@HangenYuu
Copy link
Contributor

@HangenYuu HangenYuu commented Jul 25, 2024

Description

Simplify the function class in pytensor.

  • Remove the output_subset kwargs at call time of class Function as no one is using it.
  • Check and Remove unused pytensor.function() parameters (use default value in codes instead)
  • Expose trust_input as a parameter.

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):
Copy link
Contributor Author

@HangenYuu HangenYuu left a comment

Choose a reason for hiding this comment

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

These are the current parameters to pytensor.function():

  • inputs: Iterable[Variable]
  • outputs: Variable | Iterable[Variable] | dict[str, Variable] | None = None
  • mode: str | Mode | None = None
  • updates: Iterable[tuple[Variable, Variable]] | dict[Variable, Variable] | None = None
  • givens: Iterable[tuple[Variable, Variable]] | dict[Variable, Variable] | None = None
  • no_default_updates: bool = False
  • accept_inplace: bool = False
  • name: str | None = None
  • rebuild_strict: bool = True
  • allow_input_downcast: bool | None = None
  • profile: bool | ProfileStats | None = None
  • on_unused_input: str | None = None

The ones I propose to remove and use a default value are

  • accept_inplace: False No inplace ops by default.
  • rebuild_strict: True by default.
  • allow_input_downcast: None by default.
  • profile: None by default.
  • on_unused_input: None by default.

I also want to ask about givens and no_default_updates since I have never used them before (givens seem unremoveable though).

@ricardoV94
Copy link
Member

The ones I propose to remove and use a default value are

Those are all still useful, unfortunately.

There's a getattr in call that we can remove according to the comment about being there for old pickles

@HangenYuu
Copy link
Contributor Author

Some things we could probably remove because nobody uses them:

  1. Default values for function argument

Oh wait, then what did you mean here in #552 🫤

@ricardoV94
Copy link
Member

Some things we could probably remove because nobody uses them:

  1. Default values for function argument

Oh wait, then what did you mean here in #552 🫤

https://pytensor.readthedocs.io/en/latest/library/compile/io.html#value-initial-and-default-values

@HangenYuu
Copy link
Contributor Author

Some things we could probably remove because nobody uses them:

  1. Default values for function argument

Oh wait, then what did you mean here in #552 🫤

https://pytensor.readthedocs.io/en/latest/library/compile/io.html#value-initial-and-default-values

Oh so you meant to remove value parameter from the pytensor.compile.io.In?

Copy link
Contributor Author

@HangenYuu HangenYuu left a comment

Choose a reason for hiding this comment

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

Also, I realized 2 things that output_keys will create output_subset behind the scene. So removing output_subset means that I also need to remove output_keys, together with the tests testing for them e.g., test_partial_function_with_output_keys. Is this okay?

@maresb maresb changed the title Simplify function Simplify the Function class Jul 26, 2024
@ricardoV94
Copy link
Member

Also, I realized 2 things that output_keys will create output_subset behind the scene. So removing output_subset means that I also need to remove output_keys, together with the tests testing for them e.g., test_partial_function_with_output_keys. Is this okay?

Sounds okay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants