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.

Sign up for GitHub

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

Jump to bottom

Add --extra option to ck2yaml #794

Merged
merged 5 commits into from
Feb 16, 2020
Merged

Add --extra option to ck2yaml #794

merged 5 commits into from
Feb 16, 2020

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jan 14, 2020 •
edited
Loading

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

If applicable, fill in the issue number this pull request is fixing

Addresses Cantera/enhancements#15, #339

Alternative to #777

Changes proposed in this pull request

The --extra option takes a YAML file as input and adds the following abilities:

  • specify alternate description string
  • copy (user-defined) custom fields to YAML output

Example

ck2yaml --input=gri30.inp --output=gri30.yaml \
--thermo=gri30_thermo.dat --transport=gri30_tran.dat --name=gri30 --extra=extra.yaml

with extra.yaml being

description: |-
  Updated webpage at http://combustion.berkeley.edu/gri-mech/version30/text30.html

foo:
- spam
- eggs

bar:
  spam: eggs

(the description is different from the original header due to the added last line)

While not addressed directly, references (i.e. #339) can be easily added by specifying an alternate description field in the --extra file.

@ischoegl ischoegl force-pushed the add-extra-option-to-ck2yaml branch 2 times, most recently from 2f4f49f to c8385e3 Compare January 14, 2020 17:18
@ischoegl ischoegl mentioned this pull request Jan 14, 2020
Closed
4 tasks
@ischoegl
Copy link
Member Author

ischoegl commented Jan 14, 2020 •
edited
Loading

@speth and @bryanwweber ... this PR comes close to the suggested --metadata option from #777. As a small change, I am proposing to add all fields at the top level (which allows for overwriting of auto-generated description fields).

Regarding the ck2yaml illustration example I had in #777, I decided to move things to Cantera/cantera-jupyter@3c75324 (which I'll convert into a PR once conda updates to 2.5.0a4)

@bryanwweber
Copy link
Member

bryanwweber commented Jan 14, 2020 •
edited
Loading

@ischoegl I triggered builds of the conda-recipes CI services. It should upload a new package shortly in a few hours.

Unless the builds fail...https://travis-ci.org/Cantera/conda-recipes/jobs/637035390

@bryanwweber
Copy link
Member

Hmmm... It looks like it uploaded packages (at least for Linux/Python) anyways, despite the failures. You might find that it works anyways, @ischoegl

speth
speth approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Looks good to me. We'll also want to add some information about this to the page on the website for ck2yaml, once Cantera/cantera-website#91 has been merged.

@ischoegl
Copy link
Member Author

ischoegl commented Jan 17, 2020 •
edited
Loading

@bryanwweber ... re conda packages. FYI, I just attempted to create a new conda environment, i.e.

conda create -n ctdev -c cantera/label/dev libcantera-devel cantera

but still get 2.5.0a3 (on Linux). I may be overlooking something?

Regarding the proposed Jupyter example, I can document the --extra option there in addition to the website as well.

@ischoegl ischoegl mentioned this pull request Jan 27, 2020
Merged
@ischoegl
Copy link
Member Author

@bryanwweber ... turns out that the conda packages are indeed updated, but the version in SConstruct erroneously still reads 2.5.0a3 (despite README.rst etc. saying otherwise), causing ct.__version__ to produce the outdated version number. I pushed a fix in #798.

bryanwweber
bryanwweber requested changes Feb 7, 2020
View reviewed changes
Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, thanks @ischoegl for taking this on. I have a few suggestions and a few points of clarification below.

interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/ck2yaml.py Outdated Show resolved Hide resolved
Ingmar Schoegl added 2 commits February 10, 2020 08:02
@ischoegl
[YAML] Add --extra option to ck2yaml
72c3993 
The --extra option takes a YAML file as input and adds the following abilities:
- specify alternative description string
- copy (user-defined) custom fields to YAML output
@ischoegl
[YAML] Add unit test for --extra option
1832b34
@ischoegl ischoegl force-pushed the add-extra-option-to-ck2yaml branch 2 times, most recently from f8841f8 to 6cc6b7b Compare February 10, 2020 15:45
@ischoegl
Copy link
Member Author

@bryanwweber ... I think I took care of everything. It’s now possible to recreate your hand-edits with the --extra option.

@bryanwweber
Copy link
Member

@ischoegl I made some minor formatting changes, can you just double check that everything is still what you expect? Also, you may want to configure your text editor/IDE to automatically remove trailing spaces on lines.

@codecov
Copy link

codecov bot commented Feb 16, 2020 •
edited
Loading

Codecov Report

Merging #794 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #794      +/-   ##
==========================================
+ Coverage   71.54%   71.57%   +0.02%     
==========================================
  Files         372      372              
  Lines       44348    44376      +28     
==========================================
+ Hits        31730    31760      +30     
+ Misses      12618    12616       -2
Impacted Files Coverage Δ
test/thermo/thermoFromYaml.cpp 100% <0%> (ø) ⬆️
src/tpx/utils.cpp 94.59% <0%> (ø) ⬆️
include/cantera/thermo/ThermoPhase.h 28.28% <0%> (ø) ⬆️
src/thermo/PureFluidPhase.cpp 59.72% <0%> (+0.08%) ⬆️
src/base/AnyMap.cpp 84.27% <0%> (+0.29%) ⬆️
src/thermo/ThermoPhase.cpp 69.59% <0%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b74f11b...fdee1e8. Read the comment docs.

@ischoegl
Copy link
Member Author

@bryanwweber ... looks all good to me. Thanks!

@bryanwweber bryanwweber merged commit f6b2e6e into Cantera:master Feb 16, 2020
@ischoegl ischoegl deleted the add-extra-option-to-ck2yaml branch February 22, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@speth speth speth approved these changes

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ischoegl @bryanwweber @speth