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

Paper: PyTeCK: a Python-based automatic testing package for chemical kinetic models #206

Merged
merged 31 commits into from
Aug 10, 2016

Conversation

kyleniemeyer
Copy link
Contributor

Still (very much) finishing this paper, but wanted to get current progress submitted. Will have final draft committed tonight 👍

@kyleniemeyer kyleniemeyer changed the title nearly complete version of paper Niemeyer PyTeCK paper May 31, 2016
@kyleniemeyer
Copy link
Contributor Author

Alright, this is the actual complete draft... apologies for getting it in late.

@sbenthall sbenthall changed the title Niemeyer PyTeCK paper Paper: Niemeyer PyTeCK paper Jun 9, 2016
@sbenthall sbenthall changed the title Paper: Niemeyer PyTeCK paper Paper: PyTeCK: a Python-based automatic testing package for chemical kinetic models Jun 9, 2016
@katyhuff katyhuff self-assigned this Jun 11, 2016
@kyleniemeyer
Copy link
Contributor Author

Hmm, noticed that on the PDF site it seems like my paper isn't building... strange, it worked fine on my local machine

@sbenthall
Copy link
Contributor

Noted. @sarostru any idea what may be causing the build failure?

@kyleniemeyer
Copy link
Contributor Author

@bryanwweber suggested the build error might be due to the accent in "Zsély" in the LaTeX log file, although I don't know why that particular name (and not the others with accents) would cause a problem.

@sarostru
Copy link
Contributor

I don't know exactly, I'll be able to look into it tonight. Sounds like maybe a missing font package or something.

@katyhuff
Copy link
Member

@kyleniemeyer Yeah... that's a bit weird. Your PR builds fine on my laptop. I'll review from that pdf. Maybe docutils needs to updated on the server?

@kyleniemeyer
Copy link
Contributor Author

Thanks! I've got some fixes I can push tomorrow, if you haven't started your review.

On Jun 24, 2016, at 1:07 AM, Katy Huff notifications@github.com wrote:

@kyleniemeyer Yeah... that's a bit weird. Your PR builds fine on my laptop. I'll review from that pdf. Maybe docutils needs to updated on the server?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@katyhuff
Copy link
Member

Sounds good. I can certainly wait to read it on a plane that will take off in about 28 hours. So, I'll git pull before I take off and review anything that's in the repo at that point!

@bryanwweber
Copy link

@katyhuff @sarostru I was unable to build it on my MacBook Pro with MacTeX 2016 and everything-updated conda Python. The error is here: https://github.com/scipy-conference/scipy_proceedings/blob/2016/publisher/build_paper.py#L146 when Python tries to decode the TeX log file, it fails, and if you look at the log file is has \xe9 (or something like that) in a line describing an overfull hbox warning in the references, without the finishing byte. I'm not sure why TeX (I assume its TeX's fault since its in the log file) is not properly encoding the é that @kyleniemeyer mentioned.

@kyleniemeyer
Copy link
Contributor Author

OK, @katyhuff I'm more comfortable with the state of the paper now 😁.

Also def not sure why it wouldn't build, still no problems locally.

@sarostru
Copy link
Contributor

@bryanwweber thanks for investigating that was as far as I got as well, I wasn't able to resolve the problem beyond tracking it to a Tex problem.

At this point in the process we could replace the é with an e to get it building and then resolve the problem later.

information. Mandatory datapoint elements include the initial ``temperature``,
``pressure``, and mixture ``composition``, as well as the experimental
``ignition-delay`` and ``ignition-type`` (means by which PyTeCk detects
ignition). All quantities provided include a magnitude and units, which
Copy link
Member

Choose a reason for hiding this comment

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

The options supported for ignition-type are not clear to me. That is, would "peak detection" be an allowable option? Perhaps I'm misunderstanding. Or, perhaps a little extra explanation of this model parameter is warranted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is discussed in a bit more detail on page 5; is that explanation more clear? I have added "discussed in more detail later" here.

@kyleniemeyer
Copy link
Contributor Author

Thanks @eteq and @katyhuff (and again to @thewtex)! I'll be responding to line comments that don't require any discussion with 🎉 to indicate that I've addressed them.

@eteq
Copy link

eteq commented Jul 6, 2016

Looking good, @kyleniemeyer - thanks!

Did you give some thought to using Zenodo or otherwise pointing to a specific (reproducible by the reader) version of the code?

@kyleniemeyer
Copy link
Contributor Author

kyleniemeyer commented Jul 6, 2016

Thanks @eteq! At this point I believe I've responded to all of your line comments. As for the things you mentioned in your longer review:

  • I added the spelling out of PyTeCK with the first mention of the name in text via cca6c9f
  • I do plan to submit to Zenodo for a DOI—I've mainly been waiting to get to the point of an official release, that will correspond to the final version of this paper. (I'm also one of the people helping coordinate efforts to standardize software citation practices so I'm fairly aware of the importance of citing a specific version 😛.)
  • I also plan to submit to PyPI once the pre-release version is DOI'ed. For now, I've added some install instructions to the PyTeCK README, for using either python setup.py install or pip install .
  • I absolutely plan to write some Sphinx-based docs, and host them on Read the Docs. I hope to get this completed soon after this paper is finished.

I'm pretty sure I'm happy with the software for a pre-release version, so I should get a DOI and submitted to PyPI (with associated paper updates) shortly.

@kyleniemeyer
Copy link
Contributor Author

Hi @katyhuff—I believe I responded to your line comments, most of which weren't too tricky to resolve (I think). I do still plan on adding a short example at the end.

Did my changes (or responses) clarify the confusion about the ignition type and where molar production rates come from?

@eteq
Copy link

eteq commented Jul 7, 2016

Awesome, thanks @kyleniemeyer (both for those things and the software citation article ;) I'm fully satisfied by this, so this looks good to go to me!

@katyhuff
Copy link
Member

katyhuff commented Jul 7, 2016

Thanks! Looks great to me as well.

@kyleniemeyer
Copy link
Contributor Author

Thank you! The only things left before I consider it "done" are to add the appropriate citation with DOI for v0.1 of PyTeCK, and I also want to add a nice usage example 😄.

@sarostru
Copy link
Contributor

sarostru commented Jul 9, 2016

Maybe I don't need to post here, but for consistency :)

Due to the complication of the proceedings deadline lining up with everyone's travel schedule to attend the conference we've decided to allow authors to spend an additional 2 days, that is until July 12th to complete any major changes to the submission. This may impact your enjoyment of the conference, so we hope not too many of you need to take the time. For reviewers, we ask that you read over this final document before the conference is over and post your decision here in the comments on or before July 17th.

Thanks and have a great Scipy!
Your proceedings co-chairs Scott Rostrup (@sarostru) and Sebastian Benthall (@sbenthall)

@kyleniemeyer
Copy link
Contributor Author

Still working on the usage example—I'm trying to include something real rather than a toy problem. Should be ready tomorrow or Monday!

@kyleniemeyer
Copy link
Contributor Author

OK, I've pushed what I believe is the final draft of the paper. I've added a new usage example section (with figure!), as well as a DOI for version 0.1.0 of the software.

@katyhuff
Copy link
Member

🎉

@thewtex
Copy link

thewtex commented Jul 12, 2016

👍 Well done!

@sbenthall sbenthall merged commit 5c825e9 into scipy-conference:2016 Aug 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants