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

Replication study #260

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

jbueltemeier
Copy link
Collaborator

Here is the first draft for a paper to be published in ReScience . It is based on the ReScience template.

It is currently being edited, feel free to comment on details to improve the paper.

Currently, individual chapters for the results, as well as the information on the hyperparameters used there, need to be attached.

@jbueltemeier jbueltemeier added the do NOT merge Do NOT merge this PR label Oct 8, 2021
@pmeier
Copy link
Member

pmeier commented Oct 10, 2021

Thanks a lot @jbueltemeier! Two things before I actually start to review:

  1. You pushed all the files that one needs to generate the paper, which in general is a good thing. Given that we are talking about 80 files here though, I think we should find another way. My idea is to only keep the files we manually edited / created and only download the rest of it at runtime. Are you ok with me looking into that and pushing to your branch?
  2. You stored the files in a chapters directory. Given that we only use sections in this paper, please rename the directory.

@jbueltemeier
Copy link
Collaborator Author

Thank you very much, @pmeier. If you have a good idea to improve this, feel free to push to this branch.

@pmeier
Copy link
Member

pmeier commented Oct 11, 2021

I'm unable to build the PDF. I think the whole graphics/* folder is missing in this PR.

@jbueltemeier
Copy link
Collaborator Author

This is because the folder with the result images is currently not committed, as there are more than 150 image files in the folder and changes can still take place. So I wanted to integrate it at the end, but if you already need it to build the PDF I can do it now.

@pmeier
Copy link
Member

pmeier commented Oct 13, 2021

What size of the folder are we talking about? Can we maybe provide the scripts (if they differ from our replication scripts (which the shouldn't)) and only commit the *.tex files that generate the figures? That way we could provide the images from our download server, e.g. https://download.pystiche.org/replication_paper/images.tar.gz

@jbueltemeier
Copy link
Collaborator Author

Currently the size of the folder is just over 45 MB.

@pmeier
Copy link
Member

pmeier commented Oct 13, 2021

Could you zip that and send it to me via email? I'll upload it afterwards.

@pmeier
Copy link
Member

pmeier commented Oct 13, 2021

Done. Could you push the graphics folder minus the images? I'll adjust the archive later so we can extract directly into it. On a side note: please store the images as .jpg's. I'd wager a guess that this already brings down the archive size significantly.

@pmeier
Copy link
Member

pmeier commented Oct 14, 2021

I've cleaned up the branch quite a bit. Compilation should happen through the default command the template provides, i.e. make. This currently fails (for me) and I've opened ReScience/template#25.

@pmeier
Copy link
Member

pmeier commented Oct 30, 2021

@jbueltemeier I've added the workaround from the issue above into the README.md. A fresh clone of the repository now works, but our compilation still fails. Could you fix that?

@jbueltemeier
Copy link
Collaborator Author

Thanks a lot @pmeier for the workaround.

As far as I can see, the compilation fails because a few packages and commands (mainly for reference purposes) are missing, which I have added to article.tex. The missing packages are standalone, which could be removed, but also lscape for the tables in the appendix, which I think is necessary to display the tables better. So now I would add this file (article.tex from the template) back or do you see another possibility?

@pmeier
Copy link
Member

pmeier commented Nov 10, 2021

Can't we put the imports on top of content.tex? If not, I'll send a fix to include the automatically in article.tex. Just \usepackage{standalone} and \usepackage{lscape}?

@jbueltemeier
Copy link
Collaborator Author

No this is not possible, because content.tex is inside the \begin{document} statement and the packages have to be called before. Currently only the two packages (\usepackage{standalone} and \usepackage{lscape}) are needed, but more might be needed later.

@pmeier
Copy link
Member

pmeier commented Nov 11, 2021

Let me send a fix then.

@pmeier
Copy link
Member

pmeier commented Nov 11, 2021

@jbueltemeier I've added a preamble.tex file in which you can put everything that needs to be loaded before \begin{document}. For this file to be included in the document, you need to run download.py again.

That being said, I'm still unable to build the document. The commands that I put in the README.md are the ones suggested to me in the issue above and they indeed work for the bare template. If you are able to compile the article, which commands are you using?

@jbueltemeier
Copy link
Collaborator Author

I have added the missing commands. Currently I only get two error messages. The filename of schultenhof_mettingen_bauerngarten_8__j.-h._janßen.jpg in images/gatys_et_al_2017/source/ seems to be incorrect after the download (ß is not correct) and the command \codeSWH from metadata.tex does not seem to work properly, perhaps because metadata.yaml is not yet completely filled in. Are you getting the same errors or is it just my system?

@pmeier
Copy link
Member

pmeier commented Mar 6, 2022

I've re-uploaded the images you sent me to https://download.pystiche.org/replication-paper/images.tar.gz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do NOT merge Do NOT merge this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants