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.

Sign up for GitHub

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

Jump to bottom

Two from_xml fixes and other documentation fixes #2074

Merged
merged 6 commits into from
May 31, 2022
Merged

Two from_xml fixes and other documentation fixes #2074

merged 6 commits into from
May 31, 2022

Conversation

paulromano
Copy link
Contributor

This PR has a few small fixes:

  • The Plot.from_xml_element method now sets the colors attribute. This required some changes in the colors setter to allow a map of ID to colors (before we only allowed Cell/Material objects). Closes Plot.from_xml_element doesn't grab colors from XML file #2073
  • A user noted that reading a cell from XML fails if a rotation matrix was specified. This has been fixed and I've added a corresponding test.
  • Explicitly mention units for surface coefficients in docstrings
  • Fix some links in the Material class docstring

gridley
gridley requested changes May 26, 2022
View reviewed changes
Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

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

My only request is that now that mask_components is successfully moved to/from XML in plots now, it should also be tested in addition to the color test you added. Should be just two lines of code.

@paulromano
Copy link
Contributor Author

@gridley Thanks for the review and the suggestion. I've just updated one of the unit tests to include mask_components going to/from XML.

pshriwise
pshriwise requested changes May 27, 2022
View reviewed changes
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.

Just one suggestion on an internal function, but otherwise this looks good to me!

openmc/plots.py Outdated Show resolved Hide resolved
pshriwise
pshriwise approved these changes May 28, 2022
View reviewed changes
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.

Looks good to me! Thanks @paulromano!

@paulromano
Copy link
Contributor Author

@gridley Do you have any more requests on this PR?

@gridley
Copy link
Contributor

gridley commented May 31, 2022

Nope, merging now!

@gridley gridley merged commit 0f7ae4b into openmc-dev:develop May 31, 2022
@paulromano paulromano deleted the xml-and-doc-fix branch May 31, 2022 21:43
NybergWISC pushed a commit to NybergWISC/openmc that referenced this pull request Jul 15, 2022
@paulromano @NybergWISC
Respond to @pshriwise comment on openmc-dev#2074
4c7237c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pshriwise pshriwise pshriwise approved these changes

@gridley gridley gridley 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.

Plot.from_xml_element doesn't grab colors from XML file
3 participants
@paulromano @gridley @pshriwise