Fix optional dependencies error detection and remove pybtex hidden dependency

[lorenzofavaro]

Description

The goal of this PR is to solve two problems.

• Remove optional dependencies present in the base version of PyBaMM, so that pybamm can be imported without optional dependencies installed
• Fix error detection: when an optional dependency is present in non-optional modules, we need to detect these errors and report them.

Fixes #3940

• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

[agriyakhetarpal]

Thanks, @lorenzofavaro – with the failing tests we can confirm that we are now testing the PyBaMM import correctly and properly :) I guess the next step is the one you highlighted about pybtex's import being required for PyBaMM's import, which should get revised so that it is only done when someone does pybamm.print_citations() and not when one does import pybamm. Maybe doing so is as easy as either moving self.read_citations() to Citations.print() plus moving self._reset() out of __init__ (since the docstring mentions that it is used for testing purposes, or moving the citations.register() bits), or using try-except blocks again (which would be less desirable).

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[lorenzofavaro]

Maybe doing so is as easy as either moving self.read_citations() to Citations.print() plus moving self._reset() out of init (since the docstring mentions that it is used for testing purposes, or moving the citations.register() bits), or using try-except blocks again (which would be less desirable).

Thanks @agriyakhetarpal, I tried this way but the problem was being able to handle a single instance of the object, and unit tests were failing. It seems like it is not the intended use of it, so either we change the way we use citations (adjusting all the unit tests) or we change the implementation.

I followed the second way and the solution I found was using the singleton pattern.
I also took the opportunity to add a handy api to register new citations with pybamm.register_citation(...).

Let me know if this solution is fine for you. I'm open to implement any other better ways.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (09ee982) to head (9eea94e).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3968   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          257      257           
  Lines        21245    21249    +4     
========================================
+ Hits         21157    21161    +4     
  Misses          88       88           

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

[agriyakhetarpal]

Thanks, @lorenzofavaro, this is great – just a single comment about the new API for registering citations which probably needs a revisit :)

[agriyakhetarpal]

Also, could we update the documentation for the optional dependencies section in the Contributing guide in this PR as necessary, as we had discussed?

[agriyakhetarpal]

Looks all great to me now, thanks for the fixes, @lorenzofavaro! Let's hope CI passes on this one, I have no other comments.

[agriyakhetarpal]

@all-contributors please add @lorenzofavaro for docs

[allcontributors]

@agriyakhetarpal

I've put up a pull request to add @lorenzofavaro! 🎉

[arjxn-py]

Thanks @lorenzofavaro, This looks good to me and I'd be happy to merge this seeing the priority. but -

[agriyakhetarpal]

Resolving previous conversations for now and all tests are passing – let's merge