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

Fix CylinderSector and IsogonalOctagon translations #3018

Merged
merged 6 commits into from
May 28, 2024

Conversation

yardasol
Copy link
Contributor

@yardasol yardasol commented May 22, 2024

Description

@MicahGale figured out that translations of CylinderSector objects were not working as intended in #3016.
Since I implemented the IsogonalOctagon class in the same way, I checked it and discovered the same bug applied.

Closes #3016

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) 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)

@MicahGale
Copy link
Contributor

Don't the planes also need to be rotated if the axis isn't z?

@yardasol
Copy link
Contributor Author

Don't the planes also need to be rotated if the axis isn't z?

@MicahGale I believe the planes are rotated. It's a little subtle, but the way we handle this in this funtion is by constructing the planes directly from a set of points. Those points will have the name numerical values, and which value goes to (x,y,z) depends on the axis we want the planes to be parallel to. Lines 159-161 shuffle the points accordingly. I added a comment to make this more clear:

        # Reorder the points to correspond to the correct central axis
        for p in points:
            p[:] = p[coord_map]

@MicahGale
Copy link
Contributor

MicahGale commented May 23, 2024

Oh I missed that; it's very clever. I see also that you do test for different orientations. Thanks for clarifying.

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 @yardasol! (and @MicahGale for identifying the issue)

@paulromano paulromano enabled auto-merge (squash) May 25, 2024 17:05
@yardasol
Copy link
Contributor Author

yardasol commented May 28, 2024

Looks like one of the tests is failing during the install step:

Downloading wcwidth-0.2.13-py2.py3-none-any.whl (34 kB)
ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    unknown package:
        Expected sha256 5cfa8d73acbab386b9d6ef8a1a01149fd096a21a23547f10bf0cf98d88300724
             Got        7e470faf3927f58657ea834e870db67c57845473eb243ab2178ee7a070a3eb6d

@MicahGale
Copy link
Contributor

Thanks @yardasol.

That message I think is coming from pip, and likely means that the file got corrupted while downloading. I think you could try having the job restart and see if it reoccurs.

@paulromano paulromano merged commit 3420199 into openmc-dev:develop May 28, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CylinderSector doesn't properly generate planes for off origin centers
3 participants