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

Spherical bb #2620

Merged
merged 13 commits into from
Aug 6, 2023
Merged

Spherical bb #2620

merged 13 commits into from
Aug 6, 2023

Conversation

caderache2014
Copy link
Contributor

@caderache2014 caderache2014 commented Jul 23, 2023

Description

This is a second attempt at adding the bounding box property to spherical meshes. The first (based on #2605) failed CI tests because the bounding box property itself was not added to the SphericalMesh class.

Fixes # (issue) #2605

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@caderache2014
Copy link
Contributor Author

@shimwell Hi John here's 2620. Yes it failed again. There was one line in which I wrote

bb = mesh.bounding_box()

Instead of....

bb = mesh.bounding_box

Obviously bounding_box is not callable. I fixed the code on my local repo, committed it to my branch spherical_bb and pushed to origin.

I don't see an option to either open a new pull request on the same branch nor rerun the same CI test on the updated pull request.

Should I .... make a new branch on my local repo (e.g. spherical_bb_2) and open a PR on that? There must be some way of re-running the CI tests...

-Chris

@caderache2014
Copy link
Contributor Author

@shimwell Nevermind. i think i figured out how to re-issue the PR.

I'm working on getting Docker and everything installed on my Ubuntu-partitioned MacBook I so can run these tests locally and avoid these problems in the future

@caderache2014 caderache2014 mentioned this pull request Jul 23, 2023
5 tasks
@shimwell
Copy link
Member

Looks like the tests are passing but if you still want to run the test locally here is a command

pytest tests/unit_tests/test_mesh.py 

@caderache2014
Copy link
Contributor Author

@shimwell Yes I did eventually figure that out. At first I was just navigating to /tests/ and running pytest. Since I don't have...docker and any number of necessary packages and libraries and virtual settings enabled it generate a lot of errors.
I did get docker set up on my laptop though ;-)

I've submitted another pull request for the cylindrical mesh as well.

openmc/mesh.py Outdated Show resolved Hide resolved
Comment on lines 70 to 71


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @caderache2014. A few comments below:

openmc/mesh.py Outdated Show resolved Hide resolved
openmc/mesh.py Show resolved Hide resolved
openmc/mesh.py Outdated Show resolved Hide resolved
openmc/mesh.py Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Shimwell <[email protected]>
@paulromano paulromano enabled auto-merge (squash) August 5, 2023 20:31
@paulromano paulromano merged commit cce80fe into openmc-dev:develop Aug 6, 2023
16 checks passed
@caderache2014
Copy link
Contributor Author

@shimwell @paulromano cool

stchaker pushed a commit to stchaker/openmc that referenced this pull request Oct 25, 2023
Co-authored-by: Jonathan Shimwell <[email protected]>
Co-authored-by: Paul Romano <[email protected]>
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