-
Notifications
You must be signed in to change notification settings - Fork 120
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
[ToricVarieties] Add total_space(E) #2781
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2781 +/- ##
==========================================
- Coverage 73.63% 73.61% -0.02%
==========================================
Files 455 455
Lines 64411 64419 +8
==========================================
- Hits 47427 47421 -6
- Misses 16984 16998 +14
|
X = total_space(canonical_bundle(S)) | ||
@test is_smooth(X) == true | ||
@test !is_fano(X) | ||
@test !is_complete(X) |
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.
Indention is weird here. I think you are mixing tabs and spaces. Please do not use tabs. In fact, please ideally read our notes about code formatting in OSCAR 😄
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.
Thank you very much! I corrected the commit following your remark.
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 seemed that there was still a mixture of tabs and spaces. If the editorconfig is correctly acknowledged by VSCode, pressing tab should add two white space characters. (For me, they show as two centered dots, whereas a tab is shown as an arrow to the right).
I have taken the liberty to update your branch with a change, that turns all tabs into two whitespace characters. If I did not make a mistake, this should now agree with the requirements in https://docs.oscar-system.org/dev/DeveloperDocumentation/styleguide/#Code-formatting.
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 am very sorry @mgemath , in my attempt to help I must have overwritten part (hopefully just a small part) of your desired changes. Really sorry. I just tried to fix my mistake, but could not. Maybe you immediately see what change I overwrote?
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.
Do not worry @HereAround. You were trying to correct my own mistakes. So it is me who must apologize in the first place. Anyway, it seems that you undone some of my corrections. This is the list.
file: src/AlgebraicGeometry/ToricVarieties/Proj/constructors.jl
lines: 28, 32, 39, 43, 60, 65.
file: src/exports.jl
line: 1378.
file: test/AlgebraicGeometry/ToricVarieties/proj.jl
line: 43.
You can find the changes here: https://github.com/oscar-system/Oscar.jl/compare/8eb7eb4cfc9d1aca1f80cbbe76f129b5987e6d06..b68e44e915573330cea170b104528d7a5cb8c595
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.
Thank you @mgemath. Just corrected my earlier mistake and added one more commit on top with a few minor optimizations. Let us see if the tests now pass.
8eb7eb4
to
b68e44e
Compare
b68e44e
to
8b90f53
Compare
8b90f53
to
2c74f72
Compare
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.
Thank you @gmemath. This looks good to me.
As discussed above, I adjusted the tabs vs whitespaces issue. We can discuss more on slack if this remains to be an issue. I have also done a few minor adjustments - nothing major.
Long story short: Thank you for the added functionality. Great!
Here we add the function total_space(E::ToricLineBundle...).
It return, as a toric variety, the total space of the sum of the line bundles appearing in E.