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

Update Jupyter examples #171

Merged
merged 7 commits into from
Mar 19, 2022
Merged

Update Jupyter examples #171

merged 7 commits into from
Mar 19, 2022

Conversation

speth
Copy link
Member

@speth speth commented Jan 17, 2022

@speth speth linked an issue Jan 17, 2022 that may be closed by this pull request
@bryanwweber bryanwweber self-requested a review February 20, 2022 13:21
This uses the following content from the first Markdown block:
- The title is taken from the first heading (starting with '#')
- The short description on the index page is the first non-heading
  paragraph (separated from other paragraphs following paragraphs by
  one or more blank lines)
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

@speth ... thank you for this update. I just pulled the artifacts and the main page looks great! Just some minor comments from my side ...

One thing I noticed is that one of the newer examples isn't added yet: ck2yaml_demo

Beyond, I think that locations of YAML files could also be pointed out here, i.e.

If you have Cantera and the Jupyter Notebook server installed on your local machine, simply download any Jupyter notebook by clicking the "Source" link at the top of each example page and you should be able to run it.

[As an aside, if I follow these instructions on Firefox/Fedora, I get to some cryptic looking output for the artifact when I click on 'source', although I didn't experience this on the actual website.]

Spot-checking, the data files are displayed correctly for lithium_ion_battery.ipynb, but are missing for equations_of_state.ipynb. On that note, the latter notebook also has some problems with LaTeX rendering that I didn't notice elsewhere.

On interactive_path_diagram.ipynb, the rendering of some of the headers is not pretty (likely the wrong header level).

Some of the issues I am pointing out are obviously not necessarily website related, but should be fixed by a minor update on cantera-jupyter.

@bryanwweber
Copy link
Member

Some of the issues I am pointing out are obviously not necessarily website related, but should be fixed by a minor update on cantera-jupyter.

I'm happy to add these to Cantera/cantera-jupyter#34

@speth
Copy link
Member Author

speth commented Mar 19, 2022

I fixed the missing ck2yaml example (a new category should have been added when that example was added), added directions explaining that the data files also need to be downloaded, and fixed the cases where the data files weren't being detected.

One case that I did not resolve is the input files for the ck2yaml example, which breaks the pattern of using the data directory for all input files. I think those input files should be moved to conform to the pattern of always using the data directory, but that needs to be dealt with on the cantera-jupyter repository.

I also did not see a rendering problem with equations_of_state.ipynb - could you be more specific (not that I think that has anything to do with this PR).

@speth speth requested a review from ischoegl March 19, 2022 18:19
@ischoegl
Copy link
Member

ischoegl commented Mar 19, 2022

I fixed the missing ck2yaml example (a new category should have been added when that example was added) [...]

Thank you! Example was merged very recently (after creation of this PR!), although I wrote it quite some time (2 years!) ago - Cantera/cantera-jupyter#27. Honestly didn't think about it until it came up here ...

One case that I did not resolve is the input files for the ck2yaml example, which breaks the pattern of using the data directory for all input files. I think those input files should be moved to conform to the pattern of always using the data directory, but that needs to be dealt with on the cantera-jupyter repository.

I think that fell between the cracks - changes date back way before Cantera/cantera-jupyter#33, although both PR's were merged the same day. I am hesitant to fix this now as it will mess with Cantera/cantera-jupyter#34. Imho, there should be some distinction as CK input is a separate category - I think it's fine to leave as is?

I also did not see a rendering problem with equations_of_state.ipynb - could you be more specific (not that I think that has anything to do with this PR).

I checked again today, and it may have something to do with my local OS. When viewing the downloaded artifacts yesterday, I had stray $'s in my output, but they're gone today.

Comment on lines 13 to 15
"Source" link. These data files should either be placed in a subdirectory named "data"
one level up from the directory containing the notebook file, or the notebook can be
modified to point to the location of these data files on your computer.
Copy link
Member

Choose a reason for hiding this comment

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

On second thought: would it make sense to just tell users to clone the cantera-jupyter repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think we want to put the barrier of using Git between our users and some of our most readily accessible examples.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. In this case I’m ok with merging … :shipit:

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, on second thought, I don't think it hurts to mention the option of cloning the repo as an alternative.

@bryanwweber
Copy link
Member

I fixed the missing ck2yaml example (a new category should have been added when that example was added),

Sorry, I should have caught that at review.

but that needs to be dealt with on the cantera-jupyter repository.

Will do.

@bryanwweber bryanwweber merged commit b8b8d06 into Cantera:main Mar 19, 2022
@speth speth deleted the jupyter-updates branch March 20, 2022 13:46
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.

Location of input files used in Jupyter examples is not obvious
3 participants