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

[SCons] Copy missing yaml input to LiC6_electrode sample #789

Closed
wants to merge 2 commits into from

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jan 9, 2020

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 testsamples) and unit tests address code coverage example works
  • The pull request is ready for review

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

Fixes #774

Changes proposed in this pull request

  • copy missing LiC6_electrodebulk.yaml file to sample folder

PS: a simpler solution would be to simply move the YAML file to the data folder, which would allow for similar examples to be implemented in Python/Matlab.

@bryanwweber
Copy link
Member

Thanks for catching this @ischoegl, I think this is a fine solution.

Re your postscript: As part of #768, I was wondering what we should do with input files that are used for examples. Having them in the data folder implies some strong level of endorsement by Cantera, IMO, since the name data implies (to me) that the contents are some fundamental data that you need to use for modeling. As such, I was considering whether it would make sense for some of the files there (e.g., nDodecane_Reitz.yaml) to move out of the data folder and just live with their relevant example scripts, while things like elements.xml or critProperties.xml (or their converted equivalents) could stay in data.

This is complicated for examples in multiple interfaces though, but I think it deserves some discussion. Probably that discussion should not happen in an arbitrary PR though, but in a dedicated issue on our spanky new repo for just such discussions 😃

@ischoegl
Copy link
Member Author

ischoegl commented Jan 11, 2020

@bryanwweber ... thanks for the comments. I agree on the issue of trying to not endorse input data, and instead imply that they serve for illustration purposes, I.e. leave things in the example folder whenever possible.

@bryanwweber
Copy link
Member

See Cantera/enhancements#22

@ischoegl
Copy link
Member Author

ischoegl commented Jan 11, 2020

See Cantera/enhancements#22

From that perspective, my solution in this PR is only a band-aid ... a better solution would automatically copy any YAML file that is encountered. Will have another look at SConstruct

PS: will wait for resolution of @speth’s comment regarding convenience of the data folder in Cantera/enhancements#22

@ischoegl
Copy link
Member Author

ischoegl commented Jan 13, 2020

Following up on @speth's comment in Cantera/enhancements#22, I added a description to clarify the intent of the input file and moved it to the data folder as an alternative option.

Depending on the outcome, either d771f83 or 0c0b8b4 should be dropped before merging. Edit: I removed the band-aid fix in SConstruct.

@ischoegl
Copy link
Member Author

Closing as PR is superseded by discussion in Cantera/enhancements#22

@ischoegl ischoegl closed this Jan 17, 2020
@ischoegl
Copy link
Member Author

ischoegl commented Feb 6, 2020

Reopening as this is a simple fix as the time frame for Cantera/enhancements#22 is unclear.

@ischoegl
Copy link
Member Author

@speth and @bryanwweber ... I ended up moving the input file to the generic data path without adding clutter to SConstruct.

At this point, this is a minimal fix that closes #774. I'm not sure whether Cantera/enhancements#22 will make it into 2.5.0.

@ischoegl ischoegl closed this Mar 29, 2020
@ischoegl ischoegl deleted the fix-LiC6_electrode-sample branch July 13, 2020 17:22
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.

Fix examples in samples folder
2 participants