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

Minor fixes in AnyValue and scons #899

Merged
merged 3 commits into from
Aug 20, 2020
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jul 10, 2020

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
4 tasks
@codecov
Copy link

codecov bot commented Jul 10, 2020

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.

src/base/AnyMap.cpp Outdated Show resolved Hide resolved
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 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.

src/base/AnyMap.cpp Outdated Show resolved Hide resolved
@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
Member Author

ischoegl commented Jul 11, 2020

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
Member Author

ischoegl commented Jul 11, 2020

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
Copy link
Member Author

🎉 ... 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.

Ensure that a YAML field `some-field: '12345'`
is read as a string by the C++ parser
The 'note' indent for the species N2 is not consistent with former
CTI/YAML versions; the note is also not identified as string
- Make path separators consistent on Windows post-install message
- Update list of Python example folders
@ischoegl ischoegl requested a review from speth July 11, 2020 15:38
@ischoegl ischoegl mentioned this pull request Aug 20, 2020
4 tasks
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. Sorry for the delayed review.

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants