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

Implement cocharacter and primitive Eulerian polynomials for hyperplane arrangements #35914

Merged
merged 6 commits into from
Aug 27, 2023

Conversation

tscrim
Copy link
Collaborator

@tscrim tscrim commented Jul 7, 2023

📚 Description

As defined in a recent paper https://arxiv.org/abs/2306.15556.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 7, 2023

@saliola I implemented this following your paper, but I don't get the positivity for simplicial arrangements you claim:

sage: H = hyperplane_arrangements.braid(4)
sage: H.is_simplicial()
True
sage: H.primitive_eulerian_polynomial()
6*z^3 - 7*z^2 + 2*z

Did I make a mistake my implementation?

@saliola
Copy link

saliola commented Jul 7, 2023

Check the minimal element. We use the intersection of all the hyperplanes as the minimal element, which I think this is opposite to Sage's convention (inclusion vs reverse inclusion).

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 7, 2023

I had thought of that. Sage is also using the 0 dimensional subspace as the minimal element. (Edit, adding example:)

sage: H.intersection_poset("subspace").minimal_elements()[0]
Affine space p + W where:
  p = (0, 0, 0)
  W = Vector space of dimension 3 over Rational Field

Do you have a nonsymmteric poset on hand to check?

@Bastidas-Jose
Copy link

It seems that in the example, the minimal element is the ambient space--expressed as the sum of p (the origin) and W (the ambient space itself).

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 8, 2023

Ah, you’re right, I misread it. I will fix it on Monday. Thank you.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 11, 2023

Thank you. I have fixed it and added a lot more examples.

@tscrim tscrim force-pushed the hyperplanes/eulerian_polynomial branch from 0a11033 to 9e14ef8 Compare July 11, 2023 01:21
@Bastidas-Jose
Copy link

That's great, thank you!

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 12, 2023

Thank you. @josebastidasolaya I will interpret that as a positive review. Although we now need @saliola to approve the PR request since permissions are now much more restrictive in this new workflow...

@fchapoton
Copy link
Contributor

doc does not build

[sagemath_doc_html-none] OSError: /sage/src/sage/geometry/hyperplane_arrangement/arrangement.py:docstring of sage.geometry.hyperplane_arrangement.arrangement.HyperplaneArrangementElement.primitive_eulerian_polynomial:3: WARNING: citation not found: BHS2023

@vbraun
Copy link
Member

vbraun commented Jul 16, 2023

pdf docs don't build

fix the reference myself
@tscrim
Copy link
Collaborator Author

tscrim commented Jul 17, 2023

@fchapoton Thank you for fixing it. Sorry I couldn't get to it sooner; I was traveling to the US (for FPSAC this week).

@vbraun
Copy link
Member

vbraun commented Jul 17, 2023

[sagemath_doc_pdf-none] ! Undefined control sequence.
[sagemath_doc_pdf-none] <argument> ... \in L} |\mu (B,X)| (z - 1)^{\codim 
[sagemath_doc_pdf-none]                                                   X},\end {split}
[sagemath_doc_pdf-none] l.2887 ...\mu(B,X)| (z - 1)^{\codim X},\end{split}

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 18, 2023

Sorry about that. This will fix that issue. I also added one additional parenthetical comment to the documentation specifying that the minimal element is the 0 dimensional subspace (which is opposite of Sage’s convention of being the entire space).

@tscrim
Copy link
Collaborator Author

tscrim commented Aug 17, 2023

@saliola, @kwankyu, @fchapoton Could someone approve the PR since it is a positive review?

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 17, 2023

Thank you. @josebastidasolaya I will interpret that as a positive review.

I don't know what "that" is, which was enough for you to "interpret that" as a positive review. I think you should have requested it explicitly from him.

As he did not express objection to your interpretation and now I approved this PR, I would not bother about it anymore.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 17, 2023

Though this PR has no problem code-wise, I have a little concern about its content:

The PR implements two concepts defined in a paper published in arXiv very recently (June 2023?). First I admit that I know nothing about the research field of hyperplane arrangements. But it seems to me that the two concepts, as defined in a very recent paper, have not yet been known widely in the field. Then it seems too early to get their implementations into sage. I think that what is implemented in sage should be something of general interest in the field, not only by a few readers and authors of a paper kept in arXiv for a few months.

I may be wrong as I don't know the field. Then please correct me.

@tscrim
Copy link
Collaborator Author

tscrim commented Aug 19, 2023

Though this PR has no problem code-wise, I have a little concern about its content:

The PR implements two concepts defined in a paper published in arXiv very recently (June 2023?). First I admit that I know nothing about the research field of hyperplane arrangements. But it seems to me that the two concepts, as defined in a very recent paper, have not yet been known widely in the field. Then it seems too early to get their implementations into sage. I think that what is implemented in sage should be something of general interest in the field, not only by a few readers and authors of a paper kept in arXiv for a few months.

IMO, that is a very arbitrary/ill-defined standard (I would also say it has already been a few months too) and it is contrary to Sage's goal of being a computational tool for experimental mathematics through sharing code. I don't see an argument in your post for why we should not include new and/or not-widely-known (in some definition) algorithms/computations/objects/etc. Yes, it can be difficult to change nomenclature (such as if a conflict arises), but we can still change method names if a need arises later.

@github-actions
Copy link

Documentation preview for this PR (built with commit e466d94; changes) is ready! 🎉

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 19, 2023

I did not attempt to define a standard of what can or cannot be included into Sage. I doubt anyone could.

In practice, it is all judged by the authors and reviewers of a PR. As a reviewer, I made my judgement based on the circumstances, which is not of much weight of course as I don't know the field. If another reviewer who knows the field think different and accepts the PR, I would not object.

For my judgement, I would not accept implementations of concepts not of general interest in the field.

Copy link
Contributor

@fchapoton fchapoton left a comment

Choose a reason for hiding this comment

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

ok

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 23, 2023
…mials for hyperplane arrangements

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

As defined in a recent paper https://arxiv.org/abs/2306.15556.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35914
Reported by: Travis Scrimshaw
Reviewer(s): Frédéric Chapoton, Kwankyu Lee
@vbraun vbraun merged commit e6a0d13 into sagemath:develop Aug 27, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Aug 27, 2023
@tscrim tscrim deleted the hyperplanes/eulerian_polynomial branch September 12, 2023 01:41
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.

7 participants