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

Fixes to example ipynb files I found while testing examples #22

Merged
merged 42 commits into from
May 23, 2024

Conversation

jspecht3
Copy link
Contributor

I went through all the example notebooks and corrected the errors I could find. I mostly ended up changing legacy functions and classes to the current/future supported functions.

I changed the outdated functions and classes to those with future support.
(e.g. changed function rectangular_prism to class RectangularPrism & changed class Source to class IndependentSource)

I changed the format for inputs that worked with old functions to inputs that work with current functions.
(e.g. The function openmc.plot_xs requires the reactions to be given as type dict, but was not before.)

I changed the initialization of the class EnergyGroups as EnergyGroups cannot be created without an initialization of group_edges.

I did this as a means to learn GitHub, so I apologize if my descriptions are not apt enough or if I did not write with the requisite level of formality. However, I do believe it would be beneficial if these errors/out-of-date-code were to be corrected. Any feedback would be greatly appreciated!

jspecht3 added 21 commits May 17, 2024 16:30
line 376: changed outdated hexagonal_prism function to HexagonalPrism class
ln 362: replaced outdated class Source with class IndependentSource
ln 276, 613, 1067: replaced outdated class Source with class IndependentSource

ln 1080: replaced outdated class Muir with function muir
ln 894: changed outdated function rectangular_prism to class model.RectangularPrism

ln 912: added '-' in front of 'box' as RectangularPrism is a composite surface and not a halfspace

ln 966: updated markdown information (see change in ln 977)

 ln 977: changed outdated class Source to class IndependentSource
ln 219: changed outdated class Source to class IndependentSource
ln 238: changed outdated class Source to class IndependentSource
ln 226: changed outdated class Source to class IndependentSource
ln 260: changed outdated class Source to class IndependentSource
ln 66: changed outdated function rectangular_prism to class RectangularPrism

ln 73, 74, 75: added '-' before 'box' as RectangularPrism is a composite surface and not a halfspace

ln 94, 509: changed outdated class Source to class IndependentSource
ln 117: changed outdated class Source to class IndependentSource
ln 7: changed title from 'Depletion' to 'Pincell Depletion'

ln 108: changed outdated function rectangular_prism to class model.RectangularPrism

ln 109: added '-' in front of 'box' as RectangularPrism is a composite surface and not a halfspace
ln 321/322: class EnergyGroups requires and initialization of group_edges
ln 302: changed oudated class Source to class IndependentSource

ln 321/322: class EnergyGroups requires an initialization of group edges
ln 124: changed outdated function rectangular_prism to class RectangularPrism

ln 158: added '-' in from of 'box' as RectangularPrism is a composite surface and not a halfspace
ln 323: changed outdated class Source to class IndependentSource
ln 263: changed outdated class Source to class IndependentSource

ln 280/281, 284/285: class EnergyGroups requires an initialization of group_edges
ln 300: changed outdated class Source to IndependentSource

ln 378/379 & 382/383: class EnergyGroups requires an initialization of group_edges
ln 509: changed outdated class Source to class IndependentSource
ln 917, 920, 926, 927, 933, 934: function openmc.plot_xs requires reactions to be given as type dict
ln 448: changed outdated class Source to class IndependentSource

ln 468/469: class EnergyGroups requires an initialization of group_edges
ln 186: changed outdated class Source to class IndependentSource
@pshriwise
Copy link
Contributor

Thanks a ton for this @jspecht3! I've been meaning to get to this for some time now, but have been busy with many other things. I'll have a look as soon as I can.

@jspecht3
Copy link
Contributor Author

Glad to hear that and I am happy to have helped!

@wahidluthfi
Copy link

Sorry for asking this silly question, but as of today, I can't open any example notebook from this git repo from my browser as I usually did before (last Friday). Also, this pull request is not updated in the notebook from the openmc documentation ([https://github.com/openmc-dev/openmc/wiki/Example-Jupyter-Notebooks))
Is there something on the administrator side that can solve this minor issue?

Here I am sending you some screenshots from my side
Screenshot_2024-05-20-07-02-20-862_com android chrome
Screenshot_2024-05-20-07-10-04-966_com android chrome

Note: I am reading the example notebook from my phone browser since it is a lot easier than opening my notebook on the bus. So I think the notebook itself is good but the preview function from Git Hub and the auto-update function from the main openmc documentation are affecting this issue.
Thanks

@pshriwise
Copy link
Contributor

Hi @wahidluthfi, you're right about the preview of that notebook in the github repo. This is usually because github's preview software stack has moved beyond what the current version of the notebook are compatible with.

As for the preview of the notebooks in the OpenMC wiki -- those links point to the current version of the notebooks in the main branch of this repository. They will be updated once this PR has been merged in.

@wahidluthfi
Copy link

Okay, thank you for the confirmation Patrick

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Thanks again for these updates @jspecht3! Mainly line comments from me and then we'll see how the testing goes.

jspecht3 and others added 3 commits May 20, 2024 16:28
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
jspecht3 and others added 4 commits May 20, 2024 16:30
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
Co-authored-by: Patrick Shriwise <[email protected]>
@jspecht3
Copy link
Contributor Author

@pshriwise I am glad to have been able to help! Please let me know how the testing goes.

@wahidluthfi
Copy link

Just a small update, now I can read the Jupyter Notebook from GitHub preview features without error. Regarding the repo, I think the current chain_simple.xml in this repo was missing some info regarding the Cs-136 because the depletion example didn't work. So I am using my old chain_simple downloaded a long time ago. I think there are also some pull requests regarding this issue.

@shimwell
Copy link
Member

Many thanks for keeping these upto date

We could perhaps think about adding some CI on the openmc repo that runs these notebooks whenever there is a PR on openmc repo from develop to master.

This could help ensure the notebooks match the latest stable release of openmc

I'm happy to make the proposed CI if anyone thinks it is a good idea

@pshriwise
Copy link
Contributor

Many thanks for keeping these upto date

We could perhaps think about adding some CI on the openmc repo that runs these notebooks whenever there is a PR on openmc repo from develop to master.

This could help ensure the notebooks match the latest stable release of openmc

I'm happy to make the proposed CI if anyone thinks it is a good idea

by all means!

@pshriwise
Copy link
Contributor

Sorry for pushing to your branch so much @jspecht3! There were some additional changes needed to update the CI runner to v0.14.0 and some changes needed in the sample chain file.

@jspecht3
Copy link
Contributor Author

@pshriwise Please push as many changes as you need and do not hesitate to let me know if there is something I need to do.

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

The notebooks run well with 0.14.0 now @jspecht3! Sorry again for all of the changes pushed to your branch. That's not normally how we do this, but there were a lot of changes needed to the CI environment.

I ended up removing testing for the part i and ii multi-group cross section tests for now. They require a change in OpenMOC to be pulled into the CI environment here. Once I've verified that this fix is working I'll submit another PR here to start testing those again:

mit-crpg/OpenMOC#484

So, with your approval @jspecht3, I'd like to merge this to get us up-to-date so we can continue development cleanly from there.

@jspecht3
Copy link
Contributor Author

@pshriwise You have my approval to merge this branch. Thank you for the experience, this whole process has been very enjoyable! I hope to be able to help in the future.

@pshriwise
Copy link
Contributor

@pshriwise You have my approval to merge this branch. Thank you for the experience, this whole process has been very enjoyable! I hope to be able to help in the future.

Anytime! Happy to receive contributions like these anytime 😃

@pshriwise pshriwise merged commit ca3bd85 into openmc-dev:main May 23, 2024
1 check passed
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.

4 participants