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

Internally import from unyt #2597

Closed

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented May 17, 2020

PR Summary

Quoting CONTRIBUTING.rst

  • Internally, only import from source files directly -- instead of:
    from yt.visualization.api import ProjectionPlot
    do:
    from yt.visualization.plot_window import ProjectionPlot

I think this principle could and should be applied to unyt, which we currently wrap under yt.units.
Directly importing from unyt instead of the wrapper would reduce the overhead of this wrapper: it's easier to learn the origin of a function/class if we avoid layered import. This should reduce the maintenance burden of the wrapper as well, though the primary goal is to make definitions easier to discover from everywhere else in yt/

highlight points

  • This should not remove anything from namespaces yt.units (and its children) in any way, so as not to break any downstream code.
  • Documentation (website + docstrings) remains unchanged, this is not about telling users not to use the wrapper, but only a matter of clarifying the code base.

@neutrinoceros neutrinoceros added enhancement Making something better code style Related to linting tools labels May 17, 2020
@neutrinoceros neutrinoceros marked this pull request as draft May 17, 2020 12:21
@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 3 times, most recently from f045c0c to a279693 Compare May 17, 2020 19:43
@neutrinoceros neutrinoceros added the yt-4.0 feature targeted for the yt-4.0 release label Jun 11, 2020
@neutrinoceros neutrinoceros changed the title [yt-4.0] Internally import from unyt [yt-4.0] Internally import from unyt (YTEP0037 2/6) Jun 22, 2020
@neutrinoceros neutrinoceros marked this pull request as ready for review June 22, 2020 17:21
@neutrinoceros neutrinoceros changed the base branch from yt-4.0 to master June 22, 2020 17:21
@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 3 times, most recently from 65f7832 to e9eef27 Compare June 22, 2020 22:56
@neutrinoceros neutrinoceros changed the title [yt-4.0] Internally import from unyt (YTEP0037 2/6) Internally import from unyt Jun 26, 2020
@neutrinoceros
Copy link
Member Author

This is the one proposal originally part of YTEP0037 that didn't reach consensus. It is not considered part of that YTEP any more.

@neutrinoceros neutrinoceros added the proposal Proposals for enhancements, changes, etc label Jul 17, 2020
@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 2 times, most recently from 2785b3b to f551dd3 Compare July 18, 2020 17:26
@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 6 times, most recently from c06952d to 185bc70 Compare July 23, 2020 20:34
@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 2 times, most recently from fc11d3a to dde36d4 Compare July 26, 2020 10:36
@neutrinoceros neutrinoceros force-pushed the direct_unyt_imports branch 2 times, most recently from 74f3faa to 051174c Compare July 26, 2020 14:41
…Quantity as aliases, and stop using them internally
@@ -3,7 +3,7 @@
hooks:
- id: flake8
- repo: https://github.com/timothycrosley/isort
rev: ''
rev: '5.1.4'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is in direct conflict with #2777, I'll remove this after #2777 is merged

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I'm having a hard time finding it, but we retained the display_ytarray stuff, right?

@neutrinoceros
Copy link
Member Author

you mean this ?

def display_ytarray(arr):

Yup, still there. This PR removes nothing, its one and only purpose is to replace indirect imports by direct ones.
There is indeed a handful of functionalities that are still defined in yt.units, but with this branch it is much easier to spot them.

@munkm
Copy link
Member

munkm commented Aug 11, 2020

For reviewers of this PR -- Here's a discussion we had about internally importing from unyt in the code styling ytep (links to the first comment in a thread):

yt-project/ytep#14 (comment)

@munkm
Copy link
Member

munkm commented Aug 18, 2020

I have some thoughts about doing this; I will weigh in when I get back early Sept.

@neutrinoceros neutrinoceros mentioned this pull request Sep 17, 2020
4 tasks
@munkm munkm removed the yt-4.0 feature targeted for the yt-4.0 release label Nov 13, 2020
Base automatically changed from master to main January 20, 2021 15:27
@neutrinoceros neutrinoceros added infrastructure Related to CI, versioning, websites, organizational issues, etc deprecation deprecate features or remove deprecated ones and removed code style Related to linting tools labels Jan 21, 2021
@neutrinoceros
Copy link
Member Author

Note that this project

https://github.com/sqlalchemyorg/zimports

Can be used to perform this change in a semi-automated way by

  1. replacing all imports from yt.units with star imports from unyt
  2. running zimports to expand explicit required imports

@neutrinoceros
Copy link
Member Author

If I'm ever reattempting this again, I'd rather start fresh. It's dishearthing to see how many conflicts this has been accumulating in less than a year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation deprecate features or remove deprecated ones enhancement Making something better infrastructure Related to CI, versioning, websites, organizational issues, etc proposal Proposals for enhancements, changes, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants