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

Errors introduced in 276: Rendering of figure and i, j index order #295

Closed
erget opened this issue Sep 3, 2020 · 10 comments · Fixed by #296
Closed

Errors introduced in 276: Rendering of figure and i, j index order #295

erget opened this issue Sep 3, 2020 · 10 comments · Fixed by #296
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors

Comments

@erget
Copy link
Member

erget commented Sep 3, 2020

Title

Errors introduced in 276: Rendering of figure and i, j index order

Moderator

None needed because this is clearly a simple error:

No moderator is needed because there cannot be any substantive discussion under these rules.

Requirement Summary

The figure introduced in #276 doesn't render correctly.

Furthermore, @AndersMS has noticed that the ordering in the text regarding the order of indices i and j are reversed in the figure - this should be harmonised as it was likely not intentional (@neumannd please correct if this is not right).

Technical Proposal Summary

  1. Update the figure or the CDL to match each other
  2. Fix the image reference so it renders

Benefits

All CF users - otherwise the document looks broken.

Status Quo

We have a broken image of what might be a misleading figure in the "next" draft

Associated pull request

#296

Detailed Proposal

1 of the problems here was the build process. I have fixed it and noted a further opportunity for improvement below.

There are 2 build jobs that should have been uploading images next to the compiled docs, but neither of them were. They were

  1. GitHub action "Build cf-conventions HTML and pdf"
  2. Travis after_success job that builds the docs and uploads them to gh-pages

Probably it took a while to notice this because the PDF is completely self-contained, so it might have been that maintainers (e.g. myself) were looking at that doc rather than the HTML in order to sanity check builds, and relying on proper exit codes as confirmation that all was well. Not so with HTML, which uses external assets that need to exist alongside the files. This issue wasn't specific to #276 but since images had previously not been used in the Conventions there was no reason it would have been a problem.

Both of these are now corrected.

Opportunity for improvement:

  1. First of all I find it a bit confusing to have builds on both Travis and GitHub Actions. If we ever get the time it might be a good idea to refactor this. This is especially important because the build process must be maintained ATM in 2 places, meaning I had to make changes to 2 files to fix this issue this time round.
  2. The after_success job in Travis only builds on master and release tags. Thus we only see issues like this for those tags; we may want to have an experimental build branch for Travis, as I did in an ad-hoc way, if we keep the Travis builds.

@neumannd could you give a hand since you have the source material for the figure?

@erget erget added the defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors label Sep 3, 2020
@neumannd
Copy link
Contributor

neumannd commented Sep 3, 2020

@erget Do you know why the figure does not render correctly? Is it a wrong file format?

Furthermore, @AndersMS has noticed that the ordering in the text regarding the order of indices i and j are reversed in the figure - this should be harmonised as it was likely not intentional (@neumannd please correct if this is not right).

I agree. I will correct the order.

@erget
Copy link
Member Author

erget commented Sep 3, 2020

@erget Do you know why the figure does not render correctly? Is it a wrong file format?

Haven't had time to investigate yet, but I can take a look tomorrow.

I will correct the order.

Thanks!

@neumannd
Copy link
Contributor

neumannd commented Sep 3, 2020

The other new figure also does not seem to render.

@erget
Copy link
Member Author

erget commented Sep 4, 2020

Yes, I'm seeing that no figures are rendering, it looks like there's an issue with the assets as referenced since it works when you build the docs locally.

My hypothesis is that when the built HTML is pushed to the website, the static assets don't make it. Investigating now...

@erget
Copy link
Member Author

erget commented Sep 4, 2020

Looks like this is correct, as the images make it into the PDF. So next to the i, j ordering we have a build issue here.

@neumannd
Copy link
Contributor

neumannd commented Sep 4, 2020

@erget Thanks for investigating

@erget
Copy link
Member Author

erget commented Sep 4, 2020

Analysis of build problem

I'll also insert this into the issue description above.

1 of the problems here was the build process. I have fixed it and noted a further opportunity for improvement below.

There are 2 build jobs that should have been uploading images next to the compiled docs, but neither of them were. They were

  1. GitHub action "Build cf-conventions HTML and pdf"
  2. Travis after_success job that builds the docs and uploads them to gh-pages

Probably it took a while to notice this because the PDF is completely self-contained, so it might have been that maintainers (e.g. myself) were looking at that doc rather than the HTML in order to sanity check builds, and relying on proper exit codes as confirmation that all was well. Not so with HTML, which uses external assets that need to exist alongside the files. This issue wasn't specific to #276 but since images had previously not been used in the Conventions there was no reason it would have been a problem.

Both of these are now corrected.

Opportunity for improvement:

  1. First of all I find it a bit confusing to have builds on both Travis and GitHub Actions. If we ever get the time it might be a good idea to refactor this. This is especially important because the build process must be maintained ATM in 2 places, meaning I had to make changes to 2 files to fix this issue this time round.
  2. The after_success job in Travis only builds on master and release tags. Thus we only see issues like this for those tags; we may want to have an experimental build branch for Travis, as I did in an ad-hoc way, if we keep the Travis builds.

@neumannd please let me know when you've committed your changes to the index ordering to finalise this PR. Feel free to use the PR I've already started.

@lesserwhirls
Copy link
Contributor

Opportunity for improvement:

  1. First of all I find it a bit confusing to have builds on both Travis and GitHub Actions. If we ever get the time it might be a good idea to refactor this. This is especially important because the build process must be maintained ATM in 2 places, meaning I had to make changes to 2 files to fix this issue this time round.

Last spring I made PR #251 to add the GitHub Action to make sure the doc build was checked on each PR. One reason I went with GitHub Actions over extending the existing Travis build is that it makes it relatively easy to generate previews for PRs in the form of downloadable artifacts. It would be relatively straight forward (famous last words 😃) to fully migrate the Travis build to GitHub Actions, and I would be happy to do that work.

@erget
Copy link
Member Author

erget commented Sep 7, 2020

Hi @lesserwhirls , that sounds awesome! I fully support migrating the Travis build into GitHub actions - let's move discussion of this into a separate issue I'll shard out of here.

@erget
Copy link
Member Author

erget commented Sep 7, 2020

The PR associated with this issue is now finalised and I have verified that it resolves both issues in the description. Barring objections, I will merge this PR on 2020-09-29. Thanks to all who've contributed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Conventions text meaning not as intended, misleading, unclear, has typos, format or language errors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants