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

Parameter Study Transform #532

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

Conversation

nkoskelo
Copy link
Contributor

Creates a mapper to expand a single instance DAG into a multiple independent instance DAG.

@nkoskelo nkoskelo marked this pull request as draft July 29, 2024 20:15
@nkoskelo
Copy link
Contributor Author

@inducer This is ready for the first round of review.

@nkoskelo nkoskelo marked this pull request as ready for review July 30, 2024 21:58
@nkoskelo nkoskelo marked this pull request as draft August 1, 2024 18:10
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Some thoughts from a first scroll.

pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved

# {{{ Operations with multiple predecessors.

def _mult_pred_same_shape(self, expr: Stack | Concatenate) -> tuple[ArraysT,
Copy link
Owner

Choose a reason for hiding this comment

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

Please think of a better name for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to _broadcast_predecessors_to_same_shape

same study then you need to have the same type of
:class:`ParameterStudyAxisTag`.
"""
size: int
Copy link
Owner

Choose a reason for hiding this comment

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

This information is already contained in the length of the tagged axis (via the array's shape), so I feel like having this would be redundant.

pytato/transform/parameter_study.py Outdated Show resolved Hide resolved

StudiesT = tuple[ParameterStudyAxisTag, ...]
ArraysT = tuple[Array, ...]
KnownShapeType = tuple[IntegralT, ...]
Copy link
Owner

Choose a reason for hiding this comment

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

Should use same type as Array.shape. I see no issue with parametric axis lengths for non-study axes.

test/test_pytato.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
and so the shape of :math:`\mathbf{Z}` will be
(:math:`\mathbf{orig\_shape}`, :math:`\mathbf{S1}.size`, :math:`\mathbf{S2}.size`).

A parameter study is specified in an array by tagging the corresponding axis
Copy link
Owner

Choose a reason for hiding this comment

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

You get this far in before saying what a parameter study is? 🤔

pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved
pytato/transform/parameter_study.py Outdated Show resolved Hide resolved

study_to_arrays: dict[frozenset[ParameterStudyAxisTag], ArraysT] = {}

active_studies: set[ParameterStudyAxisTag] = set()
Copy link
Owner

Choose a reason for hiding this comment

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

Set comprehension?

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.

2 participants