Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize some stuff in FTheoryTools #2478

Merged
merged 8 commits into from
Sep 6, 2023
Merged

Conversation

lkastner
Copy link
Member

@lkastner lkastner commented Jun 16, 2023

Loading isn't available yet and I still have to think about families of these models.

*** update

loading is available for Global Tate Model. We can either continue and work on the families?
or move the families to another pull request?

*** unrelated serialization changes that happen in this pull request and a note on serialization in experimental

I moved the serialization include between Oscar source and experimental. (which means I had to pull out the Laurent module and put it into source). I think that any serialization code that is written for anything in experimental
should be put into experimental.

@antonydellavecchia
Copy link
Collaborator

@thofma I moved the Laurent Module into source here. Maybe makes sense for you to take a quick look and make sure I didn't mess anything up there

@HereAround
Copy link
Member

Thank you for working on this @antonydellavecchia . I am not sure I understand why Laurent monomials (a.k.a. the Laurent module) are touched in this PR. Why is this necessary?

@thofma
Copy link
Collaborator

thofma commented Sep 6, 2023

Looks good for the Laurent things

@HereAround
Copy link
Member

HereAround commented Sep 6, 2023

I discussed with @apturner yesterday. Generally speaking, Andrew and I prefer smaller changes/more PRs (otherwise, too much could change in the meantime etc.). So we are very happy to first have the serialziation for the global tate models and have the other models (which sadly are - at least in part - still subject to change) addressed in separate PRs.

@antonydellavecchia
Copy link
Collaborator

Thank you for working on this @antonydellavecchia . I am not sure I understand why Laurent monomials (a.k.a. the Laurent module) are touched in this PR. Why is this necessary?

they are unrelated. I just had to move somethings around in the includes, and I had serialized somethings for the laurent module that was in experimental without realizing. We decided to move laurent into src to deal with it. Thats why i tagged Tommy in this pull request as well.

@antonydellavecchia
Copy link
Collaborator

I discussed with @apturner yesterday. Generally speaking, Andrew and I prefer smaller changes/more PRs (otherwise, too much could change in the meantime etc.). So we are very happy to first have the serialziation for the global tate models and have the other models (which sadly are - at least in part - still subject to change) addressed in separate PRs.

Since tests are passing, if you approve of the changes submit an approving review and we can get this merged

Copy link
Member

@HereAround HereAround left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this @lkastner and @antonydellavecchia . As I see it (and as @thofma is ok with the changes on the Laurent module), this should be good to go. We can address the serialization of the other models in a separate PR.

@HereAround HereAround merged commit 79ff056 into master Sep 6, 2023
8 of 13 checks passed
@HereAround HereAround deleted the lk/ftt/serialization branch September 6, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants