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

Hyperbolic surface triangulation 2 (new package) #8259

Open
wants to merge 83 commits into
base: master
Choose a base branch
from

Conversation

loic-dubois
Copy link
Member

@loic-dubois loic-dubois commented Jun 5, 2024

Summary of Changes

This package enables building and handling triangulations of closed orientable hyperbolic surfaces: a basic tool for any future implementation of algorithms operating on such surfaces. Functionalities are offered such as the Delaunay flip algorithm, and the construction of a portion of the lift of the triangulation in the Poincaré disk model of the hyperbolic plane. Triangulations can be generated by triangulating a convex fundamental domain in the Poincaré disk. A method is offered that generates such domains in genus two.

Release Management

@MaelRL MaelRL added Not yet approved The feature or pull-request has not yet been approved. Feature labels Jun 5, 2024
@MaelRL MaelRL added this to the 6.1-beta milestone Jun 5, 2024
@MaelRL MaelRL changed the title Hyperbolic surface triangulation 2 dubois Hyperbolic surface triangulation 2 (new package) Jun 5, 2024
@afabri
Copy link
Member

afabri commented Jun 5, 2024

/build:v0

Copy link

github-actions bot commented Jun 5, 2024

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8259/v0/Manual/index.html

@afabri

This comment was marked as outdated.

This comment was marked as outdated.

@sloriot
Copy link
Member

sloriot commented Sep 9, 2024

/force-build:v1

Copy link

github-actions bot commented Sep 9, 2024

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8259/v1/Manual/index.html

////////////////////////////////////////////////////////////////////////////////

template<class FT>
Complex_without_sqrt<FT>::Complex_without_sqrt(){
Copy link
Member

Choose a reason for hiding this comment

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

Change to a member initializer list.

////////////////////////////////////////////////////////////////////////////////

template<class FT>
FT Complex_without_sqrt<FT>::real_part() const{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FT Complex_without_sqrt<FT>::real_part() const{
const FT& Complex_without_sqrt<FT>::real_part() const{

Copy link
Member

Choose a reason for hiding this comment

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

done

void set_coefficient(int index, const Complex_number& coefficient);

// Returns the index-th coefficient
Complex_number get_coefficient(int index) const;
Copy link
Member

Choose a reason for hiding this comment

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

return const&

Copy link
Member

Choose a reason for hiding this comment

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

done

_combinatorial_map.mark(const_ccw(invaded), visited_darts_mark);
_combinatorial_map.mark(const_cw(invaded), visited_darts_mark);

Point a = positions[const_ccw(invader)];
Copy link
Member

@afabri afabri Sep 10, 2024

Choose a reason for hiding this comment

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

make points a,b, and c const&

Copy link
Member

Choose a reason for hiding this comment

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

done

@afabri
Copy link
Member

afabri commented Sep 11, 2024

/force-build:v1

Copy link

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/8259/v1/Manual/index.html

@afabri
Copy link
Member

afabri commented Sep 11, 2024

@loic-dubois @pougetma What is not offered by std::complex for that you come up with your own class ?

Factory class, whose purpose is to generate some convex domains of surfaces of genus two.
Factory class, whose only purpose is to construct random fundamental domains of
closed orientable hyperbolic surfaces. The method
make_hyperbolic_fundamental_domain_g2 constructs such a domain for a surface of
Copy link
Member

Choose a reason for hiding this comment

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

backtick the function.

Copy link
Member

Choose a reason for hiding this comment

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

done

@loic-dubois
Copy link
Member Author

loic-dubois commented Sep 11, 2024

@loic-dubois @pougetma What is not offered by std::complex for that you come up with your own class ?

We represent each complex number by the pair of its real and imaginary parts. We require that the number type of the real and imaginary parts supports the basic arithmetic operations +, -, *, /, and computes them exactly (without any rounding). But we dot not require anything else. In particular, we do not want to use square roots or things like that. This is a constraint that we chose, hoping that it would make computations faster.

So for example, if we were to use CGAL:Gmpq for the real and imaginary parts, then we could represent the complex number 1 + i, but we should not be able to compute its norm (for it is not a rational number).

But then std::complex has methods like "norm" or "arg". So I guess that std::complex, templated by CGAL:Gmpq, should not even compile?

@afabri
Copy link
Member

afabri commented Sep 12, 2024

@pougetma can you please configure your text editor that it does remove trailing white space. And remove the current WS.

@pougetma
Copy link
Member

@loic-dubois @pougetma What is not offered by std::complex for that you come up with your own class ?

We represent each complex number by the pair of its real and imaginary parts. We require that the number type of the real and imaginary parts supports the basic arithmetic operations +, -, *, /, and computes them exactly (without any rounding). But we dot not require anything else. In particular, we do not want to use square roots or things like that. This is a constraint that we chose, hoping that it would make computations faster.

So for example, if we were to use CGAL:Gmpq for the real and imaginary parts, then we could represent the complex number 1 + i, but we should not be able to compute its norm (for it is not a rational number).

But then std::complex has methods like "norm" or "arg". So I guess that std::complex, templated by CGAL:Gmpq, should not even compile?

It seems to me that std::complex is fine as a model for the concept
ComplexWithoutSqrt, (note that the "norm" is in fact the squared modulus).
We "only" have to modify the member names of our concept so that it fits the
one we need from std::complex. But the fact that sdt::complex provides more is
not a pb.

@afabri
Copy link
Member

afabri commented Sep 13, 2024

We "only" have to modify the member names of our concept so that it fits the
one we need from std::complex. But the fact that sdt::complex provides more is
not a pb.

I would suggest to switch to std::complex then. The less we have to maintain because it is offered by std:: types, the better.
With the functions to change, I guess you mean std::complex::imag(). I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGES.md not updated Feature Not yet approved The feature or pull-request has not yet been approved. Tests failing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants