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

Rename plot module to plotting #1696

Merged
merged 7 commits into from Feb 16, 2021
Merged

Rename plot module to plotting #1696

merged 7 commits into from Feb 16, 2021

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Feb 15, 2021

We now import only the plot function from the plotting module in the scipp __init__.py.
This means we can now simply do sc.plot(mydata) instead of sc.plot.plot(mydata).

The only other things in the plotting module that were being imported were some mostly unused shortcuts to various plot projections. These have now been removed.

This also fixes the mpl plots that were not showing in the binned_data notebook for example.

scipp/src/plot/plot_matplotlib.py
scipp/src/plot/plot.py
scipp/src/plot/tools.py
)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove them?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that they are not used at all?
CMake apparently finds and copies all the files in the subdirectories.
See discussion on Slack (scipp channel)

@nvaytet
Copy link
Member Author

nvaytet commented Feb 16, 2021

@OwenArnold how would this impact the CI chain we have with the ess and ess-notebooks repos?
Am i right that they are now based on the release builds of Scipp, not master?
So when we make a new release of Scipp, we should update all the ess code and notebooks?
Will this lead to a potentially long build-up of errors/inconsistencies between Scipp releases?

@OwenArnold
Copy link
Contributor

OwenArnold commented Feb 16, 2021

@OwenArnold how would this impact the CI chain we have with the ess and ess-notebooks repos?
Am i right that they are now based on the release builds of Scipp, not master?
So when we make a new release of Scipp, we should update all the ess code and notebooks?
Will this lead to a potentially long build-up of errors/inconsistencies between Scipp releases?

#1494 (comment) describes current working.

Yes correct. If you introduce breaking functionality in scipp, we don't see the effect downstream until the release is made. We could potentially have a next/future/dev branch on each of the repositories and also have pipelines that build from that using scipp\label\dev, That's quite a bit more management work. So not sure what best trade off is under the circumstances. Maybe we can build, but not publish these future packages, so we at least know to make the fix.

@SimonHeybrock SimonHeybrock marked this pull request as draft February 16, 2021 10:24
@SimonHeybrock
Copy link
Member

SimonHeybrock commented Feb 16, 2021

Do not merge right now (converted to draft). This will break the IKON20 Python course, and I don't know when/if the images will be rebuilt.

@SimonHeybrock
Copy link
Member

@nvaytet Or should we merge, fix the notebooks, and ask Torben to rebuild the image?

@nvaytet
Copy link
Member Author

nvaytet commented Feb 16, 2021

@nvaytet Or should we merge, fix the notebooks, and ask Torben to rebuild the image?

I don't really mind. I'll ask Torben if there is still time to rebuild the image.

@SimonHeybrock SimonHeybrock marked this pull request as ready for review February 16, 2021 15:19
@SimonHeybrock SimonHeybrock merged commit 6484f49 into main Feb 16, 2021
@SimonHeybrock SimonHeybrock deleted the plot_to_plotting branch February 16, 2021 15:20
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

4 participants