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

Updates AMS to V6.1 #444

Merged
merged 28 commits into from
Dec 6, 2024
Merged

Updates AMS to V6.1 #444

merged 28 commits into from
Dec 6, 2024

Conversation

ConorIA
Copy link
Contributor

@ConorIA ConorIA commented Oct 17, 2021

Replaces #348 and closes #298

Todos:

  • I have not made a great effort to make the skeleton article more "RMarkdown-y". I started replacing some of the \section entries with #, but I was having compilation errors, so needed to go back to start.
  • The AMS template says to use \twocolsig and \twocolcapsule when using twocol mode, but these are giving me undefined control sequence errors. I have commented out my conditional in template.tex, but would like to get the functionality working before this is merged #helpwanted!
  • The AMS template uses "and" before the final author name. I have used a for loop for authors (yay me), but I am not familair enough to know how to do a "if (i == length(authors)" type of statement to add the "and" before the last author entry. #helpwanted!
  • I have addressed the for loops, but I had to resort to using a for loop on the last array entry. That doesn't seem to be right, but I can't figure out how to access the values of authors/last. e.g. authors/last.name throws an error and authors/last["name"] returns "true", not "Author Eight".
  • News not updated yet because this may not make it into main for a while!

Thanks to @eliocamp as much of this was based on his work on the v5 template.

It looks like old releases of pandoc don't play nice with the pipes. For the record:

$ pandoc --version
pandoc 2.11.4
Compiled with pandoc-types 1.22, texmath 0.12.1, skylighting 0.10.2,
citeproc 0.3.0.5, ipynb 0.1.0.1

@ConorIA ConorIA changed the title Updates AMS to V5 Updates AMS to V6.1 Oct 17, 2021
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Hi!

Thanks for this and sorry for the delay. Here are a few comment we need to work out.

Overall it would be best in if the skeleton contains more R code examples and Markdown. That would help our test too.

R/article.R Outdated
@@ -88,9 +88,9 @@ amq_article <- function(
#' <https://www.ametsoc.org/ams/index.cfm/publications/authors/journal-and-bams-authors/author-resources/latex-author-info/>.
#' @export
#' @rdname article
ams_article <- function(..., keep_tex = TRUE, md_extensions = c("-autolink_bare_uris")) {
ams_article <- function(..., keep_tex = TRUE, md_extensions = c("-autolink_bare_uris", "-auto_identifiers")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to understand: why do we need to remove this extension here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I'm not sure what my reasoning was. It might be the sort of thing that is obvious if we put it back in and see what results.

R/article.R Outdated
pdf_document_format(
"ams", keep_tex = keep_tex, md_extensions = md_extensions, ...
"ams", keep_tex = keep_tex, md_extensions = md_extensions, citation_package = 'natbib', ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to make this also a parameter of the function so that it can be changed.

Previously it was not set - so I don't think natbib was used. Does it need to be used to work now ?

We should try to prevent things breaking if possible and this would change the default citation processing. We need to offer a way to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood that natbib is hard-coded in ametsocv6.1.cls

$exauthors.email$
% Credit to https://stackoverflow.com/a/67609365 for different last-author case.
\authors{
$if(authors/allbutlast)$
Copy link
Collaborator

Choose a reason for hiding this comment

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

This syntax allbutlast brings a min Pandoc requirement (at least 2.10) than we need to insure.

\centerline{(illustration here)}
\appendcaption{B1}{Here is the appendix figure caption.}
\end{figure}
\bibliography{references}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we put this in the template as we usually do with other formats ?

inst/rmarkdown/templates/ams/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
plot(1:10)
```{r setup, include=FALSE}
knitr::opts_chunk$set(fig.path = "", # AMS required
out.extra = "", # To force figure labels
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the issue exactly ?
fig_caption = TRUE in pdf_document() should be enough to have the caption 🤔

```{r,echo=FALSE,fig.cap='test the rmd output',fig.align='center',fig.width=3.17}
plot(1:10)
```{r setup, include=FALSE}
knitr::opts_chunk$set(fig.path = "", # AMS required
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is required we could probably make that part of the format function 🤔

Each table must be numbered, provided with a caption, and mentioned specifically in the text.
See below (in the .tex document) and at the end of the document for the formatting of a sample table (Table
\ref{t1}).
\section{Introduction}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We would need to change to Markdown before merging

Suggested change
\section{Introduction}
# Introduction

If we are lazy to do it manually, we can probably convert the tex file body using Pandoc directly

R/article.R Outdated
@@ -88,9 +88,9 @@ amq_article <- function(
#' <https://www.ametsoc.org/ams/index.cfm/publications/authors/journal-and-bams-authors/author-resources/latex-author-info/>.
#' @export
#' @rdname article
ams_article <- function(..., keep_tex = TRUE, md_extensions = c("-autolink_bare_uris")) {
ams_article <- function(..., keep_tex = TRUE, md_extensions = c("-autolink_bare_uris", "-auto_identifiers")) {
pdf_document_format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to put a Pandoc requirement before running the function here - the test are failing because of this mainly

rmarkdown::pandoc_available('2.10', TRUE)

@cderv cderv mentioned this pull request Nov 29, 2021
@cderv
Copy link
Collaborator

cderv commented Apr 19, 2023

@ConorIA do you plan to keep working on this or should I take over ?

Maybe AMS has also update to a newer version than 6.1.

Enquiring about the status of this PR following my latest review

@ConorIA
Copy link
Contributor Author

ConorIA commented Apr 20, 2023

Hi @cderv, sorry to have been unresponsive to your very detailed review. I find myself very short on free time these days, and haven't been writing much after finishing my thesis last fall. I would be happy to have you take over.

The good news is that v6.1 is still the current AMS template https://www.ametsoc.org/index.cfm/ams/publications/author-information/latex-author-info/

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2023

Thanks for the feedback - I'll add that to my list then. Thanks for your contribution

@cderv cderv self-assigned this Apr 20, 2023
@cderv cderv added the WIP label Apr 20, 2023
@CLAassistant
Copy link

CLAassistant commented Apr 17, 2024

CLA assistant check
All committers have signed the CLA.

@cderv
Copy link
Collaborator

cderv commented Nov 27, 2024

I did solve some of the conflict, but my last review did not get reply. I'll see if I can finish the work without the information of choice you made. It's been some time, so you may not remember why anyway.

@ConorIA
Copy link
Contributor Author

ConorIA commented Nov 28, 2024

Hi @cderv . Sorry for my lack of responsiveness. It sure has been a while since I opened this PR. To be honest, I'm not sure that there was much thinking in my work. It was mostly about hacking the new template into the old example and trying to make it work. Certainly not up to the quality. I will take a look at your review comments, but as I noted in 2023. I don't think I'll be the one to get this PR over the finish line. I not longer use R in my day-to-day, nor am I in academia anymore. Just rusty skills and pleasant memories at this point! Sorry about that.

@cderv cderv merged commit fe7347d into rstudio:main Dec 6, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Outdated AMS template
4 participants