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

Patch scipp classes on import #6

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Sep 6, 2022

This enables to use da.plot() after having imported plopp.

Comment on lines 17 to 20
from scipp import Variable, DataArray, Dataset

setattr(Variable, 'plot', plot)
setattr(DataArray, 'plot', plot)
Copy link
Member

Choose a reason for hiding this comment

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

So sc.plot would still call the old plotting, but the method the new one?

Should we consider (for now) patching only when explicitly calling a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

So sc.plot would still call the old plotting, but the method the new one?

Yes

Should we consider (for now) patching only when explicitly calling a function?

You mean, something like

def make_plopp_default():
    from scipp import Variable, DataArray, Dataset
    setattr(Variable, 'plot', plot)
    setattr(DataArray, 'plot', plot)
    setattr(Dataset, 'plot', plot)

and then calling

import plopp
plopp.make_plopp_default()

to do the patching?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is what I mean. And we should also patch scipp.plot in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Or do you plan to remove scipp.plot?

Copy link
Member Author

Choose a reason for hiding this comment

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

We also need to patch scipp.plot for dicts of DataArrays. Any good ideas for a function name? make_plopp_default isn't great.
plopp.activate()?

Copy link
Member

Choose a reason for hiding this comment

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

plopp.patch_scipp()?

setattr(Dataset, 'plot', plot)
def patch_scipp():
"""
Running this replaces the legacy `plot` function from Scipp with the plopp `plot`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Running this replaces the legacy `plot` function from Scipp with the plopp `plot`
Running this replaces the `plot` function from Scipp with the plopp `plot`

def patch_scipp():
"""
Running this replaces the legacy `plot` function from Scipp with the plopp `plot`
wrapper. This patches both the Variable, DataArray, Dataset class methods, as well
Copy link
Member

Choose a reason for hiding this comment

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

It is not a classmethod, is it?

Comment on lines +24 to +25
import scipp as sc
setattr(sc.Variable, 'plot', plot)
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with import order, or importing scipp again after calling this?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, import order doesn' seem to matter.

import scipp as sc
import plopp as pp
pp.patch_scipp()
import scipp

scipp.plot(da)

still gives the plopp plotting.

@nvaytet nvaytet merged commit 69650b9 into main Sep 7, 2022
@nvaytet nvaytet deleted the patch_scipp_classes_on_import branch September 7, 2022 08:18
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.

2 participants