Oscar.jl icon indicating copy to clipboard operation
Oscar.jl copied to clipboard

FTheoryTools: Add support for F-theory QSMs

Open HereAround opened this issue 1 year ago • 5 comments

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

HereAround avatar Feb 14 '24 12:02 HereAround

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%> (ø)

... and 47 files with indirect coverage changes

codecov[bot] avatar Feb 23 '24 17:02 codecov[bot]

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...)

HereAround avatar Apr 05 '24 12:04 HereAround

@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.

HereAround avatar Apr 05 '24 13:04 HereAround

@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.

lgoettgens avatar Apr 05 '24 13:04 lgoettgens

@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.

HereAround avatar May 01 '24 16:05 HereAround

I believe this is ready for review.

HereAround avatar May 14 '24 14:05 HereAround

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

antonydellavecchia avatar May 15 '24 08:05 antonydellavecchia

Please do that. I cannot even get the diff of this PR loaded on github as it touches too many files

lgoettgens avatar May 15 '24 08:05 lgoettgens

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?

HereAround avatar May 15 '24 09:05 HereAround

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)

HereAround avatar May 15 '24 11:05 HereAround

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)?

HereAround avatar May 17 '24 10:05 HereAround

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

antonydellavecchia avatar May 17 '24 11:05 antonydellavecchia

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?

HereAround avatar May 17 '24 11:05 HereAround

Make sure to squash merge so we don't get the files in the history

Just done. Thank you!

HereAround avatar May 17 '24 12:05 HereAround