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

Drop matplotlib dependency #1120

Merged
merged 2 commits into from
Dec 5, 2023
Merged

Drop matplotlib dependency #1120

merged 2 commits into from
Dec 5, 2023

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Dec 4, 2023

matplotlib seems unused in qibo, and only used in the examples

#814

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1dd73f7) 100.00% compared to head (84bc7e3) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1120   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           65        65           
  Lines         9059      9059           
=========================================
  Hits          9059      9059           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks. Out of curiosity, shouldn't we add matplotlib somewhere as optional? I think there are some tests for the examples in CI, but they are still passing so I guess it was installed as a dependecy of another dependency?

@scarrazza
Copy link
Member

scarrazza commented Dec 4, 2023

Dropping is fine but I agree with @stavros11 that we should add it as optional.

@alecandido
Copy link
Member Author

alecandido commented Dec 4, 2023

Thanks. Out of curiosity, shouldn't we add matplotlib somewhere as optional? I think there are some tests for the examples in CI, but they are still passing so I guess it was installed as a dependecy of another dependency?

I don't believe it should be optional, but rather it should be in the tests group.

The spirit is that:

  • optional dependencies are runtime dependencies, that might be needed by the user to execute some library functions, that are not part of the main workflow
  • groups are development dependencies, so they are used by developers but not by users (so they do not survive in the package)

Of course, a Qibo user might want to use matplotlib for something else. But that's completely independent of Qibo.

@scarrazza scarrazza self-requested a review December 4, 2023 20:47
Copy link
Member

@scarrazza scarrazza left a comment

Choose a reason for hiding this comment

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

Fine by me in the tests group. We could also add a sentence in the examples/README.md that some example require matplotlib.

It is actually a dependency of the examples, but they are usually run together with the tests. For the time being it is not worth a separate group
@alecandido alecandido merged commit c1227d8 into master Dec 5, 2023
21 checks passed
@alecandido alecandido deleted the drop-matplotlib branch December 5, 2023 05:20
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.

3 participants