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

Simplify the Function class #955

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Function class
2 participants