Oscar.jl
Oscar.jl copied to clipboard
oscar-system
ToricVarieties: Chow ring
I believe this is ready for review now.
~~Regarding the failure in the doctests, I am not sure if this is related to this PR. (On my local machine, I experience a lot of other doctest-failures, originating from other code in Oscar. But this might just be my system... The failure here complaints about a missing literature citation, which - so I believe - was not touched in this PR.)~~ Was my mistake and should be fixed now.
I have just force pushed. Many of the more "elementary" comments applied to all of the toric varieties and I tried to adjust all instances within the toric context. Points still open are:
- Chow ring(s) appearing in different places within OSCAR",
- The
@refmight be best addressed separately (cf. https://github.com/oscar-system/Oscar.jl/issues/1471).
Please ignore the failing status for the macos - 1.6 test, the doctests were in fact successful, as shown in the log, sorry about that. (see #1473)
@lkastner @fingolfin This is ready for a new round of reviews. Thank you!
@fingolfin @lkastner This is ready for the next round of reviews.
Based on the above feedback, I have:
- Tried to make the distinction between a rational equivalence class and an algebraic cycle crystal clear.
- As there are relations to closed subvarieties (not yet fully addressed in this PR, but it should be a start), I have extended on this too.
Thank you @lkastner for the many suggestions. Most of which were applied. Some require extra care, which I hope to address towards late afternoon (Philly). Also, I believe it makes sense to squash your many suggestions?
- "identically the same": Maybe you could clarify differently? Like "must be defined on the same toric variety, i.e. the same OSCAR variable" or something similar.
Yes that is meant. I just wanted to emphasize that an abstract isomorphism is not enough (at least for now) but it must be the same OSCAR variable. Your suggestion makes that clearer. So probably worth adjusting everywhere (= many many instances) within the toric varieties.
* Some explanations of functions are rather exhaustive, while others are quite short. It is not quite clear to me what distinguishes one kind of functions from the other.
@fingolfin asked for some details above, so I tried to explain those more carefully. (Rational: Whatever @fingolfin picks up upon, others might too. So the hope is, that the explanations are longer for complicated material and shorter for simple content. This is of course only a hope.)
* Since you added a lot of error messages you could think about testing them. Look for `@test_throws` in the other tests for examples.
I agree. This is why I added the error tests for the toric varieties. But it might not cover all warnings yet. Let me check in more detail later which ones are not yet covered. Then I can extend.
* At some point there should be some decision on how to handle one-line functions (not by us). Here they are all exhaustively documented which requires many times the effort to write and maintain than the implementation itself. I think this will be infeasible in the long run.
Ok.
@lkastner Up to the 3 comments above that I left open, I have integrated all of your suggestions.
Also, I believe it makes sense to squash your many suggestions?
Seems like you already did that?
Also, I believe it makes sense to squash your many suggestions?
Seems like you already did that?
Indeed, I have squashed them. (Were order 30 individual small commits otherwise. I hope you don't mind?)