Skip to content

Minor fixes in AnyValue and scons#899

Merged
speth merged 3 commits intoCantera:mainfrom
ischoegl:minor-fix-in-AnyValue
Aug 20, 2020
Merged

Minor fixes in AnyValue and scons#899
speth merged 3 commits intoCantera:mainfrom
ischoegl:minor-fix-in-AnyValue

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Jul 10, 2020 •
edited
Loading

Changes proposed in this pull request

  • ensure that a YAML field: some-field: '12345' is read as a string by the C++ parser (which currently returns an integer)
  • make C++ parser consistent with ruamel.yaml
  • Also: minor formatting update in scons output (mixed windows/linux style os.path.seps in install message on windows)

This is a minor fix that is left over after closing #881. Also cherry-picking minor fixes from #892 to ensure that fixes of existing features survive.

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

@ischoegl ischoegl changed the title Minor fix in AnyValue Minor fixes in AnyValue and scons Jul 10, 2020
@ischoegl ischoegl mentioned this pull request Jul 10, 2020
Closed
4 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 10, 2020 •
edited
Loading

Codecov Report

Merging #899 into main will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #899      +/-   ##
==========================================
+ Coverage   71.11%   71.18%   +0.06%     
==========================================
  Files         376      376              
  Lines       46201    46200       -1     
==========================================
+ Hits        32858    32887      +29     
+ Misses      13343    13313      -30
Impacted Files Coverage Δ
src/base/AnyMap.cpp 83.82% <100.00%> (+0.04%) ⬆️
test/general/test_containers.cpp 100.00% <100.00%> (ø)
src/thermo/RedlichKwongMFTP.cpp 78.04% <0.00%> (+1.55%) ⬆️
src/thermo/IdealSolidSolnPhase.cpp 66.39% <0.00%> (+6.47%) ⬆️

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 07dac7b...0ef12d6. Read the comment docs.

bryanwweber
bryanwweber reviewed Jul 10, 2020
View reviewed changes
speth
speth reviewed Jul 10, 2020
View reviewed changes
Copy link
Copy Markdown
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 basically fine to me. Could you update the commit "[scons] Small updates/fixes" to have a more descriptive message? It's not immediately obvious what the purpose of these changes is.

@ischoegl ischoegl force-pushed the minor-fix-in-AnyValue branch 3 times, most recently from d5d2916 to 206bb00 Compare July 11, 2020 00:44
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Jul 11, 2020 •
edited
Loading

Sorry for pushing again, but I noticed that the note in ch4_ion.yaml was not indented correctly (!). I stumbled over this when I tried to verify that the missing quotes I corrected here weren't an artifact produced by cti2yaml and/or ctml2yaml as the note happens to be a valid numeric value (result: both converters create the quotes, and indent correctly).

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Jul 11, 2020 •
edited
Loading

Will probably add another update for the formatting, as I just found a snippet for Anaconda on windows documenting the status before this PR

[...]
Cantera has been successfully installed.

File locations:

  applications                C:/Users/<user>/cantera\bin
  library files               C:/Users/<user>/cantera\lib
  C++ headers                 C:/Users/<user>/cantera\include
  samples                     C:/Users/<user>/cantera\samples
  data files                  C:/Users/<user>/cantera\data
  Python package (cantera)    C:\Users\<user>\anaconda3\envs\cantera-dev\Lib\site-packages
  Python samples              C:\Users\<user>\anaconda3\envs\cantera-dev\Lib\site-packages\cantera\examples
  Matlab toolbox              C:/Users/<user>/cantera\matlab\toolbox
  Matlab samples              C:/Users/<user>/cantera\samples\matlab

An m-file to set the correct matlab path for Cantera is at:

  C:/Users/<user>/cantera\matlab\toolbox\ctpath.m

scons: done building targets.

All this takes is a couple of os.path.normpaths ...

@ischoegl ischoegl force-pushed the minor-fix-in-AnyValue branch from 206bb00 to 891139b Compare July 11, 2020 05:48
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Jul 11, 2020

🎉 ... better:

[...]
Cantera has been successfully installed.

File locations:

  applications                C:\Users\ischo\cantera\bin
  library files               C:\Users\ischo\cantera\lib
  C++ headers                 C:\Users\ischo\cantera\include
  samples                     C:\Users\ischo\cantera\samples
  data files                  C:\Users\ischo\cantera\data
  Python package (cantera)    C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages
  Python samples              C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples
  Matlab toolbox              C:\Users\ischo\cantera\matlab\toolbox
  Matlab samples              C:\Users\ischo\cantera\samples\matlab

An m-file to set the correct matlab path for Cantera is at:

  C:\Users\ischo\cantera\matlab\toolbox\ctpath.m

scons: done building targets.

ischoegl added 3 commits July 11, 2020 10:17
@ischoegl
[Input] Ensure YAML strings are preserved
ca6c22e 
Ensure that a YAML field `some-field: '12345'`
is read as a string by the C++ parser
@ischoegl
[test_data] Fix formatting / typo
7c42402 
The 'note' indent for the species N2 is not consistent with former
CTI/YAML versions; the note is also not identified as string
@ischoegl
[scons] Fix formatting / update Python example folders
0ef12d6 
- Make path separators consistent on Windows post-install message
- Update list of Python example folders
@ischoegl ischoegl force-pushed the minor-fix-in-AnyValue branch from 891139b to 0ef12d6 Compare July 11, 2020 15:18
@ischoegl ischoegl requested a review from speth July 11, 2020 15:38
@ischoegl ischoegl mentioned this pull request Aug 20, 2020
Merged
4 tasks
speth
speth approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
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. Sorry for the delayed review.

@speth speth merged commit a9a3a4a into Cantera:main Aug 20, 2020
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 20, 2020

NP - I figure you took a well-deserved break 😉

@ischoegl ischoegl deleted the minor-fix-in-AnyValue branch September 3, 2020 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bryanwweber bryanwweber bryanwweber left review comments

@speth speth speth 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 @speth @bryanwweber