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

Adding YTEP-0035 about using traitlets. #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matthewturk
Copy link
Member

This YTEP describes how and why we could move to using traitlets for tasks such as visualization, with a light discussion of how to implement for data objects.

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

I like this and definitely support starting with viz. Just so I'm clear, does this mean you would be able to do p.width = (1, 'pc') instead of p.set_width(1, 'pc')?

source/YTEPs/YTEP-0035.rst Outdated Show resolved Hide resolved
@matthewturk
Copy link
Member Author

It does -- it also means that anything that is a property of the plot can be set at instantiation, and it also means in principle we could do something like:

p1 = yt.SlicePlot(ds, "x", "density")
p2 = yt.SlicePlot(ds, "y", "density")

traitlets.link( (p1, "width"), (p2, "width") )

and they would be kept in sync. While this is not as important for scripting code as it is for interface code, it is still a nice thing.

Copy link
Member

@munkm munkm left a comment

Choose a reason for hiding this comment

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

I really like this and support using traitlets in yt this way.

I'd also like to second Britton's comment about starting out with viz-specific traits. This could be a pretty big change for the whole codebase, so doing everything in smaller pieces is going to be helpful for review and for making implementation manageable.

Should we anticipate any issues with this and say, unyt quantities?

@matthewturk
Copy link
Member Author

When I've tested it in the past with what were then YTArray objects, I didn't have to make any changes or even monkey-patches to YTArray. I anticipate the same will be true of unyt arrays.

@matthewturk
Copy link
Member Author

So, we're at 2 approvals -- what would you two suggest for trying to move this forward?

@matthewturk
Copy link
Member Author

For what it's worth, I sat with @samwalkow and we started working on implementing bits of this, and I've pushed to my branch traitlets here: https://github.com/matthewturk/yt/tree/traitlets

The way we broke it up was to start converting a straightforward class like FRB and then build out the traitlets support as it goes. This will set it up so that the traitlets_support.py module can be cherry-picked or transplanted, and then class-by-class we can convert. This branch is not even issued as a WIP PR because we're still attempting to figure out the least-disruptive way of doing this, and how to set it up for the most productive and useful set of code reviews.

@munkm
Copy link
Member

munkm commented Oct 7, 2019

This branch is not even issued as a WIP PR because we're still attempting to figure out the least-disruptive way of doing this, and how to set it up for the most productive and useful set of code reviews.

I appreciate that you're thinking about how to streamline code review for something like this. Your initial approach sounds good, IMO. If we have time in the next triage meeting (or in a different relevant meeting) it might be worth talking through how to plan out a big change like this. I think it'd be helpful for all of us in our future development efforts (so we can learn how to do it too).

@neutrinoceros
Copy link
Member

I just want to emphasise, since I haven't found any direct documentation stating this, that mypy and traitlets seem to be compatible since IPython uses both.

@matthewturk
Copy link
Member Author

Hey folks, I know it has been a really long time since I've done anything on this PR. I want to continue it, but for the present time I'm going to focus on a PR in tandem that looks at type annotating, which will take precedence since it is a less invasive change.

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