Rename issubset to is_subscheme for subschemes (#3202)

[paemurru]

The issubset functions where the arguments are not schemes were (obviously) not renamed. This change was rather tricky to implement because

• sometimes U stands for a scheme and we need is_subscheme, sometimes U is an inverted_set and we need issubset,
• the functions do not have the output type written out in the docstring, sometimes being unclear whether the output is a scheme,
• some functions are type unstable, so that trying a simple example did not help either,
• Julia uses dynamic typing so that the errors (which are only type errors in this case) only come out when running the lengthy tests.

[simonbrandhorst]

Thanks for the effort. I appreciate it!

[lgoettgens]

Can you add deprecations for all renamed functions to src/Deprecations.jl?

[simonbrandhorst]

At some point before 1.0 is_subset will be implemented again with a different meaning (one of the reasons for renaming is_subset to is_subscheme here).
This will be a breaking change. Would this still go via a deprecation first?

[lgoettgens]

Ah ok, in this case I wouldn't do a deprecation.
If you want to use the same name again, you need to make sure that there is a Oscar release somewhere between the deletion of the old function (or rename here) and the introduction of the new function. Otherwise, users of Oscar just get different results without any way of an error

[fingolfin]

This is not fault of this PR, but I think this code is too clever for its own good and should be changed; don't do an assignment in what looks like a logical expression, one could mistake the = for a ==. Instead do

Suggested change

	check && (is_subscheme(Z, Y) || (Z = intersect(Y, Z)))
	if check && !is_subscheme(Z, Y)
	Z = intersect(Y, Z)
	end

However, more seriously, I also wonder why it does that -- normally I would expect a failed check to trigger an error, not to "correct" the problem. I can't think of any other place we do this, and indeed in this PR multiple other checks are modified that all do an error (but I do not claim to have checked all place, neither in this PR nor in all of Oscar)

[paemurru]

Actually, at least according to Julia docs, it is perfectly fine to use assignment together with short-circuit evaluation:

julia> true && (x = (1, 2, 3))
(1, 2, 3)

About check: we can take the preimage of any set by first taking the intersection with the codomain, so it looks OK to me. The user has the option to set check to false if they know that Z is a subscheme and they wish to avoid checking the containment.

[thofma]

I don't think the question was whether this is valid julia code or not.

It is a question about how much time someone has to spend to understand this, when the code has to be improved/rewritten/fixed in a few years, and the original author is not available.

[fingolfin]

As Tommy explained, this is not about what is valid syntax, this is about what is maintainable.

Moreover, check everywhere else means "raise an error if the input is invalid", and here it now means something different. That's bad. For example, before one could set check=true while e.g. debugging to verify one has no bug in the code, and if there is no error, one has evidence for that, while if there is a bug, one may see an error and can debug it.

But here check silently papers over mistakes. So presumably no error will be raised, and the behavior may even differ between check=true and check=false. I don't think that's acceptable.

But of course this is not an issue with this PR, it is an issue with the existing code, so it can be fixed in a separate PR. But fixed it should be.

[paemurru]

I completely agree. The name of the variable (check) should clearly indicate what it is checking (check_z_contained_in_y). And there should be consistency whether the function returns an error whenever the check fails. It might be worth elaborating on this in the OSCAR styleguide (it only briefly mentions input sanity checks).

[lgoettgens]

The only thing blocking this right now for me is the broken CI.

[codecov]

Codecov Report

Merging #3252 (a7fd5d7) into master (4b3ed0e) will increase coverage by 0.13%.
Report is 4 commits behind head on master.
The diff coverage is 80.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3252      +/-   ##
==========================================
+ Coverage   81.63%   81.77%   +0.13%     
==========================================
  Files         546      546              
  Lines       73163    73850     +687     
==========================================
+ Hits        59730    60393     +663     
- Misses      13433    13457      +24     

Files	Coverage Δ
experimental/Schemes/CoveredScheme.jl	94.61% <100.00%> (ø)
...etry/Schemes/AffineSchemes/Morphisms/Attributes.jl	79.66% <100.00%> (ø)
...ry/Schemes/AffineSchemes/Morphisms/Constructors.jl	100.00% <100.00%> (ø)
...cGeometry/Schemes/AffineSchemes/Objects/Methods.jl	95.00% <100.00%> (ø)
src/AlgebraicGeometry/Schemes/Gluing/Methods.jl	95.52% <100.00%> (ø)
...metry/Schemes/ProjectiveSchemes/Objects/Methods.jl	80.36% <100.00%> (ø)
...raicGeometry/Schemes/SpecOpen/Morphisms/Methods.jl	87.02% <100.00%> (ø)
...ebraicGeometry/Schemes/SpecOpen/Objects/Methods.jl	88.03% <100.00%> (ø)
.../AlgebraicGeometry/Schemes/SpecOpen/Rings/Types.jl	100.00% <100.00%> (ø)
experimental/Schemes/Sheaves.jl	64.66% <87.50%> (ø)
... and 8 more

... and 12 files with indirect coverage changes