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

[Draft] use algebraic number for reals #153

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

Conversation

bobot
Copy link
Contributor

@bobot bobot commented May 11, 2023

ocaml-flint version 0.3 is needed

@bobot
Copy link
Contributor Author

bobot commented May 12, 2023

@Gbury can you take a look at f5700bc to see if the way and place the new construct have been introduced is correct. Note that it is not anymore (root-of-with-ordering (p_0 p_1 ... p_n) i) but (root-of-with-ordering (coeffs p_0 p_1 ... p_n) i) in order to respect the syntax of terms.

Copy link
Owner

@Gbury Gbury left a comment

Choose a reason for hiding this comment

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

The addition of the new primitive looks fine (modulo some renaming as per my comment).

src/standard/builtin.ml Outdated Show resolved Hide resolved
@Gbury
Copy link
Owner

Gbury commented May 23, 2023

Looking at the CI, and I'm wondering: is calcium not available on windows ?

@bobot
Copy link
Contributor Author

bobot commented May 24, 2023

I have not made the effort of making it work there, but I have also not actively forbid it so I don't yet understand.

@bobot
Copy link
Contributor Author

bobot commented May 24, 2023

Are the package updated in the windows CI? Perhaps this should be added: opam repo add upstream https://opam.ocaml.org --rank 2 --all-switches --set-default

@bobot
Copy link
Contributor Author

bobot commented May 26, 2023

I think it is just because the original windows repo stop updating last year: https://ocaml.org/docs/ocaml-on-windows#opam-repository-mingw . I haven't been able to test adding the current one in my virtual machine. (ocaml-ci can also be a good CI since it is the one used by opam) (EDIT: sorry for the repetition, github slowly updated the page 🤦 )

@Gbury
Copy link
Owner

Gbury commented May 26, 2023

yeah, it's not the first time the windows CI has had problems because the repo used by setup-ocaml for windows runner is old and not updated regularly... I'll try and see what solutions there are (including manually adding an up-to-date repo as you suggested)

@Gbury Gbury added this to the 0.9 milestone Jun 6, 2023
@Gbury Gbury removed this from the 0.9 milestone Jun 19, 2023
@Gbury Gbury force-pushed the algebraic_number branch 2 times, most recently from be89444 to 5331850 Compare June 27, 2023 15:32
Copy link
Owner

@Gbury Gbury left a comment

Choose a reason for hiding this comment

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

I pushed a few changes, mainly some refactoring and documentation, as well as proper errors instead of the invalid_arg. The code is good for merging, but we need to figure out how to make CI runs succeed; the installation of flint/calcium takes way too long: 6h+ on the windows CI which end up being forcefully shut down before completion. Ideally, the flint/calcium/arb packages could try and make use of depexts to rely on systems binary packages for the big C (or C++ ?) libraries.

@Gbury
Copy link
Owner

Gbury commented Oct 4, 2023

ocaml/opam-repository#24509 might solve the CI problems. Will try again once it is merged.

bobot and others added 9 commits October 16, 2023 16:06
Tentative introduction of root-of-with-order

Model support algebraic number

   root-of-with-order and root-of-with-enclosure

   Error path should be improved

[Model] Set version of calcium needed

Accept root-of-with-ordering

[Algebraic] accept also root-of-with-interval

[Algebraic] accept "-3" coefficient for experiment
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.

2 participants