Skip to content

Improve normal_cone to support multiple vertices as input, and provide an option for calculating outer or inner normal cones.#5610

Merged
benlorenz merged 6 commits intooscar-system:masterfrom
atahan2000:master
Dec 10, 2025
Merged

Improve normal_cone to support multiple vertices as input, and provide an option for calculating outer or inner normal cones.#5610
benlorenz merged 6 commits intooscar-system:masterfrom
atahan2000:master

Conversation

@atahan2000
Copy link
Copy Markdown
Contributor

This is the first time I am doing it so I am open to any tips.

The things I have done:

  1. Improved the normal_cone in PolyhedralGeometry to include multiple vertices like in polymake.
  2. Putting an option to change from calculating outer to inner normal cones.
  3. Edited the documentation above the function.

… vertices like in polymake.

2. Putting an option to change from calculating outer to inner normal cone.
3. Edited the documentation above the function.
Copy link
Copy Markdown
Member

@lkastner lkastner left a comment

Choose a reason for hiding this comment

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

Thank you, this will be a valuable contribution. Once the tests run I will have another look.

Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
@atahan2000
Copy link
Copy Markdown
Contributor Author

I manually changed the things you have said, I am not sure whether there is an easier way to do it but, looking forward to your new suggestions!

Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
atahan2000 and others added 2 commits December 3, 2025 16:52
Co-authored-by: Lars Kastner <lkastner@users.noreply.github.com>
@fingolfin
Copy link
Copy Markdown
Member

Thank you!

Unfortunately code formatting is off, this can be fixed by running julia etc/format_code.jl in the Oscar.jl directory (and then committing the resulting changes).

Also, may I suggest that you use the "Resolve conversation" on any discussion you already addressed? That makes it a bit easier to follow along (I did it for some this morning, but more things have been changed by now)

@fingolfin fingolfin changed the title Changes on normal_cone Improve normal_cone to allow specifying multiple vertices, and permit choosing between calculating outer and inner normal cones Dec 3, 2025
@fingolfin fingolfin added enhancement New feature or request topic: polyhedral geometry Issue concerns polyhedral geometry code release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Dec 3, 2025
@fingolfin
Copy link
Copy Markdown
Member

I've modified the title to be in a form usable for our release notes; but I don't know enough about this topic to decide if it is sensible, so please double-check and edit it as needed.

2. Also added a condition to check whether any element in F is out of range
3. Changed the length of F condition to "Face index out of range"
@atahan2000 atahan2000 changed the title Improve normal_cone to allow specifying multiple vertices, and permit choosing between calculating outer and inner normal cones Improve normal_cone to allow multiple vertices for input, and an option for calculating outer or inner normal cones. Dec 4, 2025
@atahan2000 atahan2000 changed the title Improve normal_cone to allow multiple vertices for input, and an option for calculating outer or inner normal cones. Improve normal_cone to support multiple vertices as input, and provide an option for calculating outer or inner normal cones. Dec 4, 2025
@atahan2000
Copy link
Copy Markdown
Contributor Author

Thanks for all the help and patience. I have only book-tests (nightly) error in the checks now. Didn't understand much about it, can you help me.

@fingolfin
Copy link
Copy Markdown
Member

The booktest failures with Julia nightly are "normal" (see #5592); in general any tests against Julia normal can fail, and you don't need to worry about that (basically only those tests marked as "Required" are, well, required :-))

@fingolfin fingolfin requested a review from lkastner December 5, 2025 11:03
@atahan2000
Copy link
Copy Markdown
Contributor Author

I also rewrote the title like you said, its just rewording but felt more natural this way. Thank you for your patience, this was my first commit. Looks everything fine now then?

Copy link
Copy Markdown
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

Can you also add a few testcases in the proper testsuite (in addition to the doctest), e.g. below this one:

nc = normal_cone(square, 1)
@test nc isa Cone{T}
@test rays(nc) == [[1, 0], [0, 1]]

These will run in more different configurations than the doctests.

Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/Polyhedron/standard_constructions.jl Outdated
1. Added testcases for normal_cone with list input and outer option.
2. Changed doc to include the option outer
3. Restricted Abstract Vector to integers 64
4. Deleted the requirement of number of elements of the face.
@benlorenz benlorenz closed this Dec 9, 2025
@benlorenz benlorenz reopened this Dec 9, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.07%. Comparing base (131f5bf) to head (0d2d575).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5610      +/-   ##
==========================================
+ Coverage   84.06%   84.07%   +0.01%     
==========================================
  Files         744      744              
  Lines      101320   101373      +53     
==========================================
+ Hits        85170    85229      +59     
+ Misses      16150    16144       -6     
Files with missing lines Coverage Δ
...edralGeometry/Polyhedron/standard_constructions.jl 97.38% <100.00%> (+<0.01%) ⬆️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@atahan2000
Copy link
Copy Markdown
Contributor Author

⚠️ Report is 13 commits behind head on master.

Do I have to do anything about it?

Copy link
Copy Markdown
Member

@lkastner lkastner left a comment

Choose a reason for hiding this comment

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

Thank you.

@benlorenz
Copy link
Copy Markdown
Member

⚠️ Report is 13 commits behind head on master.

Do I have to do anything about it?

No, this is fine. Also the two failures on nightly are tracked in other issues.

@benlorenz benlorenz merged commit a3f6b0d into oscar-system:master Dec 10, 2025
101 of 112 checks passed
@atahan2000
Copy link
Copy Markdown
Contributor Author

Thank you all for the feedbacks and help! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: polyhedral geometry Issue concerns polyhedral geometry code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants