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

Fix Bibtex rendering in reports #1001

Merged
merged 24 commits into from
Jan 28, 2024
Merged

Conversation

eurunuela
Copy link
Collaborator

@eurunuela eurunuela commented Nov 22, 2023

Closes #986

Changes proposed in this pull request:

  • The PR adds a new function _bib2html() that converts the contents of the bibtex file to an HTML string.
  • The PR adds two dependencies to deal with the bibtex to HTML conversion.

@eurunuela
Copy link
Collaborator Author

eurunuela commented Nov 22, 2023

Can we please add @martaarbizu and @martinezeguiluz as contributors?

Edit: I'll do it once we merge.

Copy link

codecov bot commented Nov 22, 2023

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.61%. Comparing base (1c3f93e) to head (ead5da7).
Report is 63 commits behind head on main.

Files with missing lines Patch % Lines
tedana/reporting/html_report.py 97.14% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1001      +/-   ##
==========================================
+ Coverage   89.54%   89.61%   +0.07%     
==========================================
  Files          26       26              
  Lines        3395     3428      +33     
  Branches      619      625       +6     
==========================================
+ Hits         3040     3072      +32     
  Misses        207      207              
- Partials      148      149       +1     

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

Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @eurunuela I noticed that the references are properly rendering at the bottom of the report, but the citations in the "About Tedana" section are still not rendering (see screenshot). Looking at your edits, I'm not sure you directly address the in-text citations. Should that be left for a separate PR or addressed here?

Also, this is minor, but, with the current rendering, it might look nicer with a line of whitespace between each full reference.
image

@eurunuela
Copy link
Collaborator Author

The main issue is the citekey for Hunter 2007. I updated the bib file but the tests are still failing but I don't know why. @tsalo @handwerkerd any idea why?

@notZaki
Copy link
Contributor

notZaki commented Jan 26, 2024

@eurunuela Does the key for Hunter need to be updated at tedana/workflows/tedana.py L858?

Edit: Technically the line in docs/output.rst L549 is also using the old key

@eurunuela
Copy link
Collaborator Author

Ah good catch @notZaki! I forgot we hardcoded the references. Just updated those files.

@notZaki
Copy link
Contributor

notZaki commented Jan 26, 2024

@eurunuela I was playing around with it a bit. Sent a PR to your branch. Might fix some stuff, might break other things. 🎲

@eurunuela
Copy link
Collaborator Author

eurunuela commented Jan 26, 2024

Thank you @notZaki! I didn't really try to optimize the code. It was a quick PR, so thank you for tackling that. I don't have access to my laptop until Sunday. I'll merge then.

Minor changes/fixes to bibtex rendering
@eurunuela
Copy link
Collaborator Author

It looks great now, thank you @notZaki!

This is ready for review.

Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you to @eurunuela and @notZaki for isolating and fixing this issue!

Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

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

Go for it - looks good to me, text seemed to render correctly! nice work per usual

@eurunuela eurunuela merged commit b6c53fc into ME-ICA:main Jan 28, 2024
16 checks passed
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.

Fix reference rendering in HTML report
6 participants