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

Add inconsistencies keyword to derived_from #9192

Merged
merged 6 commits into from Jun 22, 2022

Conversation

avriiil
Copy link
Contributor

@avriiil avriiil commented Jun 16, 2022

This is a work in progress.
This PR does not yet build correctly.

As part of the discussion in #9177

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@quasiben
Copy link
Member

add to allowlist

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @rrpelgrim! I see this is marked as a draft PR. Often this means someone is working on something but it may not currently be in a state were other people should take time to review. Is this PR where you would you like a review, or is that premature?

@avriiil
Copy link
Contributor Author

avriiil commented Jun 16, 2022 via email

expand=True`` with unknown ``n`` will raise a ``NotImplementedError``
@avriiil avriiil marked this pull request as ready for review June 16, 2022 17:29
@avriiil
Copy link
Contributor Author

avriiil commented Jun 16, 2022

@jrbourbeau - this is ready for review

Copy link
Member

@pavithraes pavithraes left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @rrpelgrim! I looked at this with @ncclementi, and this is close!

test_ufuc_meta is failing because it checks for docstring similarity with numpy, after removing the Dask specific disclaimers. It'll be tricky to include the inconsistencies in the disclaimer test because it's a parameter we pass to derived_from.

Taking a step back though, we probably don't want to always display "Known inconsistencies: None" because it might suggest that there are no inconsistencies. It'll be nicer to display it only if there are any inconsistencies included. The code we've suggested is implementing this, and they seem to work locally. :)

Also, the linting tests are failing, you can run the pre-commit hooks to fix that locally and then commit: https://docs.dask.org/en/stable/develop.html#code-formatting

dask/utils.py Outdated Show resolved Hide resolved
dask/utils.py Outdated Show resolved Hide resolved
No "Known inconsistencies" shown when inconsistencies=None
@avriiil
Copy link
Contributor Author

avriiil commented Jun 21, 2022

@pavithraes - I accepted your suggestions and ran the pre-commit hooks. LMK if this looks OK now?

@ncclementi
Copy link
Member

@rrpelgrim it looks like when running the pre-commit hooks there was a file that wasn't added to the commit: accessor.py. See https://github.com/dask/dask/runs/6985825841?check_suite_focus=true for reference, can you run the pre-commit again and include the changes to that file?

Excluding linting, the resto LGTM.
@jrbourbeau I think this is ready for final review.

@ncclementi
Copy link
Member

There is a failure on a test, but it seems unrelated https://github.com/dask/dask/runs/7006605767?check_suite_focus=true#step:6:21930

@jsignell since James is out for a couple of weeks, would you be able to give this PR a final review?

Copy link
Member

@jsignell jsignell left a comment

Choose a reason for hiding this comment

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

This looks great! For posterity: here is what the docstring looks like rendered:

image

@jsignell jsignell merged commit 8323ee9 into dask:main Jun 22, 2022
@avriiil avriiil deleted the add-inconsistencies-to-derived-from branch June 23, 2022 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants