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

Implements dpctl.tensor._flags #921

Merged
merged 5 commits into from
Oct 10, 2022
Merged

Implements dpctl.tensor._flags #921

merged 5 commits into from
Oct 10, 2022

Conversation

ndgrigorian
Copy link
Collaborator

@ndgrigorian ndgrigorian commented Oct 3, 2022

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?

Reworks usm_ndarray.flags to return a dpctl.tensor._flags object, which streamlines checking flags, such as array memory order and if it's writable.

e.g., for an array X, X.flags now prints the truth values of the currently implemented flags: C-contiguous, F-contiguous, and Writable, while X.flags.forc returns True if one of C-contiguous and F-contiguous is true, and False otherwise.

Note: this will fail the flake8 style guide check as flake8 does not recognize 'cimport' in _flags.pyx as valid syntax.

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

@coveralls
Copy link
Collaborator

coveralls commented Oct 3, 2022

Coverage Status

Coverage increased (+0.02%) to 82.115% when pulling 74043c3 on flags-helper-class into 72daccc on master.

@ndgrigorian ndgrigorian force-pushed the flags-helper-class branch 2 times, most recently from 719e117 to 9d1e830 Compare October 4, 2022 18:06
@oleksandr-pavlyk
Copy link
Collaborator

Can you please either merge main branch into this PR, or rebase it on top of the main branch to fix the CI?

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_142.

@oleksandr-pavlyk
Copy link
Collaborator

@ndgrigorian The linter failed. Is pre-commit passing in your local workflow?

@ndgrigorian
Copy link
Collaborator Author

@ndgrigorian The linter failed. Is pre-commit passing in your local workflow?

Yes, but it skips flake8. When I run flake8 on _flags.pyx, it thinks cimport is a syntax error.

_dlpack.pxd fails flake8 runs for the same reason, but pre-commit passes it as well, because it skips flake8.

@diptorupd
Copy link
Contributor

@ndgrigorian The linter failed. Is pre-commit passing in your local workflow?

Yes, but it skips flake8. When I run flake8 on _flags.pyx, it thinks cimport is a syntax error.

_dlpack.pxd fails flake8 runs for the same reason, but pre-commit passes it as well, because it skips flake8.

@ndgrigorian Please add the following entry for the _flags.pyx in the per-file-ignore section of the .flake8 config.

dpctl/tensor/_flags.pyx: E999, E225, E226, E227

@oleksandr-pavlyk We need to document how to handle flake8 related config for pyx files it in the CONTRIBUTING document.

@github-actions
Copy link

github-actions bot commented Oct 9, 2022

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_151.

@github-actions
Copy link

github-actions bot commented Oct 9, 2022

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_151.

@oleksandr-pavlyk
Copy link
Collaborator

I have made changes warranting the use of Cython:

  • class Flags is now a Cython class, i.e. cdef class Flags: instead of class Flags:
  • class has two typed members, usm_ndarray arr_ and int flags_
  • wrote typed bit checking routine, and used it to implement properties
  • implemented __eq__ method

Also modified the test file to leverate __eq__, and to add tests to covert each property/method of Flags class.

@oleksandr-pavlyk
Copy link
Collaborator

@ndgrigorian Please look over the changes and merge.

@ndgrigorian
Copy link
Collaborator Author

@ndgrigorian Please look over the changes and merge.

I've committed a small change: Numpy's FNC flag is F-contiguous-and-not C-contiguous, so I moved the previous implementation of fnc to a new property, fc, and changed fnc to match Numpy's. I also adjusted the tests to reflect that.

@github-actions
Copy link

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_152.

ndgrigorian and others added 3 commits October 10, 2022 10:10
… __eq__

Support for `__eq__` allows to compare two instances of Flags objects and
compare Flags object to an integer.
@github-actions
Copy link

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_157.

@github-actions
Copy link

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_156.

@oleksandr-pavlyk
Copy link
Collaborator

Given my confusion of the meaning of Flags.fnc, perhaps property docstrings are in order. They wont be user accessible, since Cython does support this yet, but it would be inspectable through source code. This can be filed as an issue and added in a separate pr.

@oleksandr-pavlyk oleksandr-pavlyk merged commit 8cbed99 into master Oct 10, 2022
@oleksandr-pavlyk oleksandr-pavlyk deleted the flags-helper-class branch October 10, 2022 20:21
@github-actions
Copy link

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

@github-actions
Copy link

Array API standard conformance tests failed to run for dpctl=0.14.0dev0=py310h8c27c75_156.

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.

4 participants