-
Notifications
You must be signed in to change notification settings - Fork 3
Add a check for inner_outer convention #51
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
base: master
Are you sure you want to change the base?
Conversation
| try: | ||
| import shapely | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this to the dependencies and always do this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It felt a bit overkill to require shapely for such a simple check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add shapely as a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it's not a huge dependency, so may as well?
zoidberg/poloidal_grid.py
Outdated
| inner = shapely.Polygon(zip(R[2], Z[2])) | ||
| outer = shapely.Polygon(zip(R[-2], Z[-2])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these slices be ranges? Or are we really just interested in R[2], Z[2]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just the inner and outer boundary should be checked.
Of course we could check all slices, and make sure they are monotonically increasing, but I think just those two should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant that these really look like scalars -- what shape does R[2] have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R and Z are 2D, thus R[2] is a 1D array of a contour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, then I would prefer to write this as R[2, ...], which makes it much clearer
|
R is 2D, so R[2] is 1D, I.e. the inner line
…On 2 December 2025 5:54:14 pm GMT+01:00, Peter Hill ***@***.***> wrote:
@ZedThree commented on this pull request.
> + inner = shapely.Polygon(zip(R[2], Z[2]))
+ outer = shapely.Polygon(zip(R[-2], Z[-2]))
Sorry, I meant that these really look like scalars -- what shape does `R[2]` have?
--
Reply to this email directly or view it on GitHub:
#51 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
No description provided.