-
Notifications
You must be signed in to change notification settings - Fork 81
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
Finalize and build pharmaceuticals manuscript #848
Conversation
AppVeyor build 1.0.3142 for commit 4cb0b6d is now complete. Found 0 potential spelling error(s). Preview:... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One missing period!
Co-authored-by: Casey Greene <cgreene@users.noreply.github.com>
AppVeyor build 1.0.3145 ... Found 0 potential spelling error(s). Preview:for commit 2013fbb is now complete. The rendered manuscript from this build is temporarily available for download at: |
Figures are not showing up in the docx build. @vincerubinetti is it possible the changes to rootstock could be involved in this? I can't think of anything else we've done that would have affected it, but I also can't check whether this broke with the update to rootstock because the docx we were building at that time doesn't have any figures! |
By the changes, you mean the plugin refactor from a couple days ago? manubot/rootstock@87c1195 All of the changes in that PR should only affect the html output, as those files only get inserted in the html build: I'm at a loss for why the docx would be different now. |
Thanks @vincerubinetti, it was the plugin refactor -- here it was #840 and I saw a couple of files outside the plugins directory were changed (e.g., ci/deploy.sh), so I was hoping there might be some crossover! I might manually fix this for now because I'm so far behind on getting this out to authors, but I'm curious if @agitter or @cgreene has any of ideas of what might have changed with the docx output? It's got something to do with rendering pictures in Word, I think, because some of them are static and some are dynamic yet none of them are showing up. We know it was still working at #819, which was the last time we built a docx with a figure in it. |
My specific rootstock PR didn't touch anything outside of the plugins directory, so perhaps it's been a while since this manuscript pulled from upstream and there have been other rootstock changes that I'm not aware of in that time (like |
I would guess this has more to do with general issues with svg images in docx outputs than any recent rootstock changes. There is some discussion of this in the abandoned pull request manubot/rootstock#40 The final comments there hint that svg images in docx works on some systems but not our CI build environments. Using png images in docx outputs is reliable. I can help brainstorm general solutions. However, I probably won't get to it until Thursday or Friday. If you want to get this out to co-authors, manually fixing the images this time may be a reasonable workaround. |
Ah I didn't see that these were SVG files. I didn't think Word really supports SVGs that well. I'm surprised it ever worked at all. |
I checked the docx artifact (pharmaceuticals-manuscript.docx), and the problem is only the svg images. We have .png versions of those already generated. We could switch to the .png versions instead of the .svg temporarily or permanently. The issue with svg images in docx is a general pandoc issue: Figure 3 in the docx has extra whitespace at the bottom that could be cropped before the final version is submitted. The Unicode characters don't show up correctly in the docx either. For example, when I view the docx I see: |
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>
Thanks @agitter, I just noticed that I was wrong about which figure was which! That's great that it's just the svgs. And thank you for catching my oversight on the contributions! Whitespace issue I can address, and I actually think that the Unicode issue will be an easy fix. It seems like I mostly added the ones that were in the pathogenesis section, which means they were done with html formatting (e.g., |
AppVeyor build 1.0.3146 for commit 18b4f3b is now complete. Found 0 potential spelling error(s). Preview:... |
AppVeyor build 1.0.3147 for commit 9ec2327 is now complete. Found 0 potential spelling error(s). Preview:... |
@agitter I think I fixed these issues except that I'm not sure where the specification for the file extension for the dynamic figures is provided, since the syntax in the document looks like this-- Grep isn't helping me figure out which script is pulling the associated image. Do you have any thoughts on where this happens in the pipeline? |
The file extensions are hidden in too many layers of indirection to find easily. They are being specified in .json files on the I pushed fd68382 to the |
Commenting here in addition to gitter, the links in references 210 and 211 (Synairgen) seem to 404, can anyone confirm? They have a nonsensical headline ("people") as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-ran the GitHub Actions build and the .png images show up in the docx now. They look okay in the PDF and HTML but a little pixelated. I can try increasing the dpi later.
Commenting here in addition to gitter, the links in references 210 and 211 (Synairgen) seem to 404, can anyone confirm? They have a nonsensical headline ("people") as well.
I plan to do a final read through Wednesday or Thursday and will try to remember to fix this. We can use a manual reference to set the titles and switch to the Web Archive URL.
Thank you @cbrueffer and @agitter for catching that! I believe I was looking at press releases on their website about a week ago, so it's odd that they're already 404-ing. I think the first one can be found here: https://www.globenewswire.com/news-release/2020/03/18/2002574/0/en/Synairgen-to-start-trial-of-SNG001-in-COVID-19-imminently.html And the second here: https://www.icpcovid.com/sites/default/files/2020-08/200720-Synairgen-annmounces-positive-results-from-trial-of-SNG001-in-hospitalised-COVID-19-patients.pdf I'm not sure why the links from their own website no longer work and I don't know what the best way to handle this would be. I'm going to merge this in but I'll open an issue based on @cbrueffer's comment! |
[ci skip] This build is based on 69f3717. This commit was created by the following CI build and job: https://github.com/greenelab/covid19-review/commit/69f37172049d7a763351a7682833fa81b1eb8ae9/checks https://github.com/greenelab/covid19-review/runs/553164739
[ci skip] This build is based on 69f3717. This commit was created by the following CI build and job: https://github.com/greenelab/covid19-review/commit/69f37172049d7a763351a7682833fa81b1eb8ae9/checks https://github.com/greenelab/covid19-review/runs/553164739
Description of the proposed additions or changes
This PR:
Related issues
#738
Suggested reviewers (optional)
Checklist