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

Issue 1182 verbose outputs and tagging PyBaMM citations #2862

Merged
merged 11 commits into from
Apr 13, 2023

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Apr 6, 2023

Description

Adds a caller method for citations.py to tag PyBaMM citations to their use of particular models or solvers and adds verbose output while printing citations

Fixes #1182

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 6, 2023

@rtimms this implementation differs from what I had thought about doing initially with a metaclass and @register_citation, but it does seem to work as intended – now that I see it, @brosaplanella's idea of adding a table for even simpler output (#1182 (comment)) can be done too (though probably in a different PR?)

Note: tests fail due to CasADi, will merge develop after they pass

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @agriyakhetarpal !

@agriyakhetarpal
Copy link
Member Author

Ready to merge once tests pass

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c714f59) 99.69% compared to head (a460035) 99.69%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2862   +/-   ##
========================================
  Coverage    99.69%   99.69%           
========================================
  Files          273      273           
  Lines        19071    19083   +12     
========================================
+ Hits         19012    19024   +12     
  Misses          59       59           
Impacted Files Coverage Δ
pybamm/citations.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@valentinsulzer
Copy link
Member

why did you skip ci?

@agriyakhetarpal
Copy link
Member Author

In the last commit I only added comments to improve coverage, I did not know that skipping CI this way would not run the coverage workflow

@agriyakhetarpal
Copy link
Member Author

I'll do a force push to re-run them if that would be okay

@agriyakhetarpal agriyakhetarpal force-pushed the issue-1182-tag-citations branch from 95ddd80 to fc1bfad Compare April 7, 2023 00:14
@valentinsulzer
Copy link
Member

Yeah you have to run all the tests to get CI, it's usually worth doing anyway unless you really did only add extra tests. Still need to fix one line of coverage

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 7, 2023

Thanks, I fixed the coverage – tag_citations() is available as a standalone method as of now but can be set to private later since it is wrapped within the current print methods too.

Though I don't know how to fix no-method-argument (E0211) on Codacy, because changing it from _caller_name() to _caller_name(self) will not run it on the same code object included in the outer frame but execute it on itself, which breaks the intended functionality with inspect.stack (I tested on ipython and I was having issues, it was not tagging the correct class). Otherwise this should be fine to merge now

@rtimms
Copy link
Contributor

rtimms commented Apr 13, 2023

thanks @agriyakhetarpal ! I'm happy to merge even if codacy isn't happy. looks like failing test is random and will be fixed by #2844

@rtimms rtimms merged commit 7c69f1d into pybamm-team:develop Apr 13, 2023
@agriyakhetarpal agriyakhetarpal deleted the issue-1182-tag-citations branch April 13, 2023 15:39
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.

Tag PyBaMM citations
3 participants