search

inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

move pytato array context from meshmode #14

Closed matthiasdiener closed 3 years ago

matthiasdiener commented 3 years ago

Originally at https://github.com/inducer/meshmode/pull/116

inducer commented 3 years ago

@matthiasdiener Could you please revert a1b97a6?

matthiasdiener commented 3 years ago

@matthiasdiener Could you please revert a1b97a6?

sure, done

inducer commented 3 years ago

get rid of most of meshmode e9156d5

:+1: Thanks!

matthiasdiener commented 3 years ago

I think this is ready for another review @inducer @kaushikcfd . I had to work around some test failures, maybe you have ideas for better fixes.

kaushikcfd commented 3 years ago

add pytato to dependencies

I think we should follow pyopencl and not make pytato a dep.

matthiasdiener commented 3 years ago

add pytato to dependencies

I think we should follow pyopencl and not make pytato a dep.

Packages using arraycontext now need pytato to run the tests; I'm not sure how to better handle this.

inducer commented 3 years ago

This has merge conflicts. Could you take a look?

matthiasdiener commented 3 years ago

This has merge conflicts. Could you take a look?

We'll have to implement ravel now for pytato I guess, but could you please take a look at the rest?

inducer commented 3 years ago

We'll have to implement ravel now for pytato I guess, but could you please take a look at the rest?

That's actually pretty complicated. (@kaushikcfd, correct? How is pytato's reshape support?) I would recommend overriding the norm instead. The ravel test can be xfailed.

matthiasdiener commented 3 years ago

We'll have to implement ravel now for pytato I guess, but could you please take a look at the rest?

That's actually pretty complicated. (@kaushikcfd, correct? How is pytato's reshape support?) I would recommend overriding the norm instead. The ravel test can be xfailed.

I implemented a version of ravel() (that passes the test), but not sure if it's correct.

Edit: If it is correct, we could move _rec_ravel to pytato itself probably.

kaushikcfd commented 3 years ago

Supporting ravel fully (including the order "K" and "A") requires some thought, essentially it asks the question: "What is the order/strides of a pytato array expression's dims?". order = "C" for non-parametric shapes is fully supported till now. order="F" is not difficult to support, but order="K" and "A" are ill-defined for current pytato trunk.

inducer commented 3 years ago

Supporting ravel fully (including the order "K" and "A") requires some thought, essentially it asks the question: "What is the order/strides of a pytato array expression's dims?".

https://github.com/inducer/pytato/pull/92. IOW: IMO, that question is not supposed to have a well-defined answer.

matthiasdiener commented 3 years ago

I think this is officially ready for review.

inducer commented 3 years ago

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

matthiasdiener commented 3 years ago

This is ready for review I think.

kaushikcfd commented 3 years ago

This is ready for review I think.

I still need to take care of one comment. I will update here, once that's done.

kaushikcfd commented 3 years ago

This is ready to go in from end too!

matthiasdiener commented 3 years ago

Ready for review @inducer.

The followup PR with meshmode's version of transform_loopy_program is here: https://github.com/inducer/meshmode/pull/216

kaushikcfd commented 3 years ago

For anyone wondering about the CI failures: https://github.com/inducer/meshmode/pull/238 should fix those.

inducer commented 3 years ago

Thanks all for seeing this through. Looks great! In it goes.