Oscar.jl
Oscar.jl copied to clipboard
FTheoryTools: Add support for F-theory QSMs
Goal: Support the F-Theory QSMs and their known properties.
Note: The change looks excessive since I added a massive json file with cool data. (Even more, this file was not created by me but by @antonydellavecchia , see https://github.com/antonydellavecchia/polytope-csv-parsing/blob/master/test.json).
cc @antonydellavecchia @apturner
Codecov Report
Attention: Patch coverage is 50.00000%
with 60 lines
in your changes are missing coverage. Please review.
Project coverage is 81.26%. Comparing base (
2a7a92b
) to head (684ae3e
). Report is 21 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #3363 +/- ##
==========================================
- Coverage 81.35% 81.26% -0.09%
==========================================
Files 577 580 +3
Lines 78646 79161 +515
==========================================
+ Hits 63983 64331 +348
- Misses 14663 14830 +167
Files | Coverage Δ | |
---|---|---|
...eoryTools/src/Serialization/hypersurface_models.jl | 100.00% <100.00%> (+8.45%) |
:arrow_up: |
.../FTheoryTools/src/LiteratureModels/constructors.jl | 89.14% <43.75%> (-3.01%) |
:arrow_down: |
src/Serialization/containers.jl | 87.66% <39.02%> (-7.76%) |
:arrow_down: |
...ental/FTheoryTools/src/Serialization/qsm_models.jl | 54.38% <54.38%> (ø) |
I have just completed the steps that we discussed in gathertown on Tuesday. So @antonydellavecchia , I believe this is ready for you to take over and complete the next step.
(Also rebased, and hopefully just fixed a silly doctest bug that I introduced a few minutes ago...)
@lgoettgens I see failures from Aqua.jl
in https://github.com/oscar-system/Oscar.jl/actions/runs/8570358046/job/23488124796?pr=3363. If I recall correctly, you mentioned during this week that this a bug elsewhere and is being investigated? It is of course not impossible that this PR causes this, but I would find it surprising. Most of the (excessively large) changes here are from adding files for a data base and read in data from them in one way or another.
@lgoettgens I see failures from
Aqua.jl
in oscar-system/Oscar.jl/actions/runs/8570358046/job/23488124796?pr=3363. If I recall correctly, you mentioned during this week that this a bug elsewhere and is being investigated? It is of course not impossible that this PR causes this, but I would find it surprising. Most of the (excessively large) changes here are from adding files for a data base and read in data from them in one way or another.
No, that's a different issues that I introduced in a new AA release about an hour ago. I'll make a fix asap.
@antonydellavecchia
- The error "encountered a non-effective model section" was a genuine bug. I am providing the fix in https://github.com/oscar-system/Oscar.jl/pull/3668.
- There were more issues that prevented a loop over all QSM models. In all those cases, this traced back to ill-formatted data in QSMDatabase.json. I have fixed those formatting issues in said .json file. I am not sure if for instance QSMDatabase.mrdi needs to be adjusted?
In any case, once the above fix is merged (if you are curious, you could just cherry-pick this change), the following should work:
indices = [parse(Int, replace(filename, ".mrdi" => "")) for filename in readdir("Models/QSMDB")]
[literature_model(arxiv_id = "1903.00009", model_parameters = Dict("k" => i)) for i in indices]
On my laptop, this takes roughly 30 minutes to complete.
I believe this is ready for review.
So we have managed to get the size down quite a bit, but we are still look at about > 30mb of storage, so we should still be using an artifact
Please do that. I cannot even get the diff of this PR loaded on github as it touches too many files
So we have managed to get the size down quite a bit, but we are still look at about > 30mb of storage, so we should still be using an artifact
Fine with me. However, I am not sure how/what to do for such an artifact. Can you look into this? Maybe best if we discuss on slack?
In testing this PR with @HechtiDerLachs this morning, I found a hopefully minor issue that needs fixing. I am not (yet) sure what caused it, but hopefully this can be fixed in the existing serialization @antonydellavecchia?
Take the following example (but it most likely also happens for all other QSM models, not just the one with k = 283
):
qsm_model = literature_model(arxiv_id = "1903.00009", model_parameters = Dict("k" => 283));
H = qsm_model.hs_model
Then we find:
julia> cox_ring(ambient_space(H))
Multivariate polynomial ring in 14 variables over QQ graded by
x1 -> [1 0 0 0 0 0 0 0 0]
x2 -> [0 1 0 0 0 0 0 0 0]
x3 -> [0 0 1 0 0 0 0 0 0]
x4 -> [0 0 0 1 0 0 0 0 0]
x5 -> [0 1 0 -1 0 0 0 0 0]
x6 -> [1 0 0 0 0 0 0 0 0]
x7 -> [-2 1 1 1 0 0 0 0 0]
x8 -> [0 0 0 0 1 0 0 0 0]
x9 -> [0 0 0 0 0 1 0 0 0]
x10 -> [0 0 0 0 0 0 1 0 0]
x11 -> [0 0 0 0 0 0 0 1 0]
x12 -> [0 0 0 0 0 0 0 0 1]
x13 -> [0 0 0 0 1 1 0 -1 -2]
x14 -> [0 0 0 0 0 1 1 1 1]
However, there are chosen names for the variables in this toric variety, as we can for instance see from the following:
julia> parent(hypersurface_equation(H))
Multivariate polynomial ring in 14 variables over QQ graded by
x1 -> [1 0 0 0 0 0 0 0 0]
x2 -> [0 1 0 0 0 0 0 0 0]
x3 -> [0 0 1 0 0 0 0 0 0]
x4 -> [0 0 0 1 0 0 0 0 0]
x5 -> [0 1 0 -1 0 0 0 0 0]
x6 -> [1 0 0 0 0 0 0 0 0]
x7 -> [-2 1 1 1 0 0 0 0 0]
v -> [0 0 0 0 1 0 0 0 0]
e3 -> [0 0 0 0 0 1 0 0 0]
e2 -> [0 0 0 0 0 0 1 0 0]
u -> [0 0 0 0 0 0 0 1 0]
e4 -> [0 0 0 0 0 0 0 0 1]
e1 -> [0 0 0 0 1 1 0 -1 -2]
w -> [0 0 0 0 0 1 1 1 1]
Indeed, those are the variable names that we want, i.e. there should be u, v, w, e1, e2, e3, e4 appearing. Even more,
the test parent(hypersurface_equation(H)) == cox_ring(ambient_space(H))
is required to give true
.
Here is a hack to achieve this:
h = hypersurface_equation(H)
S = cox_ring(ambient_space(H))
var_names = symbols(parent(h))
S.R.S = var_names
H.hypersurface_equation = Oscar.eval_poly(string(h), S)
This looks promising now in that the doctests complete successfully. Thank you @lgoettgens !
There are few things to be fixed, e.g. the issue mentioned above about the variables in the cox ring. I believe this could be fixed within this PR and the serialization @antonydellavecchia ?
There are a few other changes: the literature_model
constructor should return the hs_model with attributes, some of which are meta-data in the corresponding .json file, some others are special for the QSMs such as the dual graph etc. Maybe it is best if I address those changes in a separate PR (since this PR has already been sitting here for 3 months and is now almost ready to go)?
This looks promising now in that the doctests complete successfully. Thank you @lgoettgens !
There are few things to be fixed, e.g. the issue mentioned above about the variables in the cox ring. I believe this could be fixed within this PR and the serialization @antonydellavecchia ?
There are a few other changes: the
literature_model
constructor should return the hs_model with attributes, some of which are meta-data in the corresponding .json file, some others are special for the QSMs such as the dual graph etc. Maybe it is best if I address those changes in a separate PR (since this PR has already been sitting here for 3 months and is now almost ready to go)?
the changes to the variables has already been made you might need to fetch the new artifact if your still receive the wrong ring
This looks promising now in that the doctests complete successfully. Thank you @lgoettgens ! There are few things to be fixed, e.g. the issue mentioned above about the variables in the cox ring. I believe this could be fixed within this PR and the serialization @antonydellavecchia ? There are a few other changes: the
literature_model
constructor should return the hs_model with attributes, some of which are meta-data in the corresponding .json file, some others are special for the QSMs such as the dual graph etc. Maybe it is best if I address those changes in a separate PR (since this PR has already been sitting here for 3 months and is now almost ready to go)?the changes to the variables has already been made you might need to fetch the new artifact if your still receive the wrong ring
Great. So then maybe we can bring this in as a first working version and I fine tune in a separate PR?
Make sure to squash merge so we don't get the files in the history
Just done. Thank you!