search

oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
338 stars 120 forks source link

Roadmap 1.0 and beyond #3054

Open fieker opened 10 months ago

fieker commented 10 months ago

Deadlines

Plan

Kaiserslautern: two regular meetings:

Things critical for the book (end of January!!)

Examples in the book should match what we get in 1.0 even though 1.0 won't be finished in time for the book deadline. That means a few items should be done before; and if we can't do them, then we may have to "lie" in the book in a few examples (i.e. edit output to match what it is supposed to be in 1.0, not what it is right now)

Things critical for 1.0 (e.g. to avoid breaking semver in 1.1)

Important for 1.0 in the sense of "should be there to look good", but not in a technical sense (not breaking semver)

These are things we want for the grant application reviewers, but technically they are separate from 1.0

Important but could be deferred to 1.x:

YueRen commented 10 months ago

Regarding combinatorics in OSCAR, it would be nice if the following types would be more "consistent" relative to itself and each other:

With "consistent" I vaguely mean the following (will update if something new springs to my mind):

fieker commented 10 months ago

We should also look at some foundational discussions in Nemo/ AA:

lkastner commented 10 months ago 
* all constructors above are named after their types, while the constructor that creates a graph from an incidence matrix is called [`graph_from_adjacency_matrix`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#graph_from_adjacency_matrix)

There are two "incidence matrices" for a graph, one encodes which edge contains which vertices, and the other encodes which vertices are adjacent. This command takes the second kind, which still has the input type IncidenceMatrix, nevertheless in this case it encodes the adjacency.

* the commands [`nv`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#nv-Union%7BTuple%7BGraph%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BDirected,%20Undirected%7D) and [`ne`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#ne-Union%7BTuple%7BGraph%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BDirected,%20Undirected%7D) should be renamed to `n_vertices` and `n_edges`  to be more consistent with `n_vertices`, `n_rays`, etc.

We are not trying to be consistent with these commands, we are trying to be consistent with what is in Graphs.jl. Furthermore @lgoettgens already added nedges and nvertices for graphs:


julia> g = complete_graph(4)
Undirected graph with 4 nodes and the following edges:
(2, 1)(3, 1)(3, 2)(4, 1)(4, 2)(4, 3)

julia> nedges(g) 6

julia> nvertices(g) 4


Once there is `GraphsBase.jl` we will use this to derive our graphs from it. The commands  `n_vertices`, `n_edges` and `n_rays` seem not to exist.

>     * Both `PolyhedralComplex` and `PolyhedralFan` have [`rays_modulo_lineality`](https://docs.oscar-system.org/stable/PolyhedralGeometry/polyhedral_complexes/#rays_modulo_lineality-Union%7BTuple%7BPolyhedralComplex%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BFloat64,%20FieldElem%7D), but the corresponding function for vertices in `PolyhedralComplex` and `Polyhedron` is called [`minimal_faces`](https://docs.oscar-system.org/stable/PolyhedralGeometry/polyhedral_complexes/#minimal_faces-Union%7BTuple%7BPolyhedralComplex%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BFloat64,%20FieldElem%7D) and not `vertices_modulo_lineality`

I'd rather rename `rays_modulo_lineality` to something more sensible, but `minimal_faces` did not make sense for a fan, since this would consist of only the lineality space.
fieker commented 9 months ago

sort out some of the mutating operators, esp for generic matrices. They are not really inplace... und unneccessarily so

lkastner commented 9 months ago

Would it make sense to have a milestone for 1.0?

YueRen commented 9 months ago

And would it make sense to have a coding sprint before the release of 1.0?

Regarding the names, I personally like the *_modulo_lineality names, since I find it descriptive and both gfan and polymake do things modulo lineality space by default (i.e., $F->RAYS will not return something empty just because $F has lineality).

lgoettgens commented 9 months ago

finalize type and function names (at least any user facing)

What about more or less weirdly types in Hecke that currently do not fit into our naming scheme, e.g. NfRelNS, GrpAbFinGen or AlgMat?

thofma commented 9 months ago

I added https://github.com/oscar-system/Oscar.jl/issues/1914 to the list, which I consider critical. The orderings of ideal gens are basically broken.

YueRen commented 9 months ago

The plan mentions two meetings in Kaiserslautern, what are the dates?

jankoboehm commented 9 months ago

1914 was in principle fixed in #2109 which segfaulted, the fix was then implemented in #2646, #2670, #2673. The last case is fixed in #2732, which at the time still segfaulted in Singular. Some analysis was done in the internals, but not conclusive. This is probably still the current state.

thofma commented 9 months ago

I tried it out today and the example still gave the wrong result. So I am not sure what exactly "is fixed", but it seems that the original example is still broken.

jankoboehm commented 9 months ago

The use of assure was wrong in a lot of instances, all of which have been fixed, except one, which is think is indeed the one triggered in the original issue. This is fixed in #2732 which triggers a segfault in Singular. @fingolfin and @hannes14 looked into what could be the issue, but so far I think the problem is still there. I can look into it again, since in the meanwhile there were changes.

micjoswig commented 6 months ago

For some weird reason, I did not see this discussion before. Let me comment briefly, even if 1.0 is complete now.

With "consistent" I vaguely mean the following (will update if something new springs to my mind):

* [`polyhedral_fan`](https://docs.oscar-system.org/stable/PolyhedralGeometry/fans/#polyhedral_fan) and [`subdivision_of_points`](https://docs.oscar-system.org/stable/PolyhedralGeometry/subdivisions_of_points/#subdivision_of_points-Union%7BTuple%7BT%7D,%20Tuple%7BType%7BT%7D,%20Union%7BMatElem,%20AbstractMatrix%7D,%20IncidenceMatrix%7D%7D%20where%20T%3C:Union%7BFloat64,%20FieldElem%7D) are incidence matrix **last**, while [`polyhedral_complex` ](https://docs.oscar-system.org/stable/PolyhedralGeometry/polyhedral_complexes/#polyhedral_complex) is incidence matrix **first**

That's probably unresolved, and that's bad.

* all constructors above are named after their types, while the constructor that creates a graph from an incidence matrix is called [`graph_from_adjacency_matrix`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#graph_from_adjacency_matrix)

This function takes any matrix and interprets the thing as an adjacency matrix; so that's correct. Adjacency refers to node vs node, whereas the incidence matrix of a graph is node versus edge. In particular, the former is always square, whereas the latter is not, in general. The data type IncidenceMatrix is appropriate for both. Sorry if this is confusing. I do not know of a good way to avoid this.

* the commands [`nv`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#nv-Union%7BTuple%7BGraph%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BDirected,%20Undirected%7D) and [`ne`](https://docs.oscar-system.org/stable/Combinatorics/graphs/#ne-Union%7BTuple%7BGraph%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BDirected,%20Undirected%7D) should be renamed to `n_vertices` and `n_edges`  to be more consistent with `n_vertices`, `n_rays`, etc.

Done. But nv and ne are kept for graphs for compatibility with standard graph packages in Julia.

* Both `PolyhedralComplex` and `PolyhedralFan` have [`rays_modulo_lineality`](https://docs.oscar-system.org/stable/PolyhedralGeometry/polyhedral_complexes/#rays_modulo_lineality-Union%7BTuple%7BPolyhedralComplex%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BFloat64,%20FieldElem%7D), but the corresponding function for vertices in `PolyhedralComplex` and `Polyhedron` is called [`minimal_faces`](https://docs.oscar-system.org/stable/PolyhedralGeometry/polyhedral_complexes/#minimal_faces-Union%7BTuple%7BPolyhedralComplex%7BT%7D%7D,%20Tuple%7BT%7D%7D%20where%20T%3C:Union%7BFloat64,%20FieldElem%7D) and not `vertices_modulo_lineality`

The term "minimal face" is standard in optimization textbooks. Hence it is chosen here. This is in line with the global decision to pick function names according to textbooks.