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

Reenable Nemo coeff rings for singular #73

Closed
wants to merge 7 commits into from

Conversation

sebasguts
Copy link
Contributor

@wbhart
This is a first try to get Nemo coefficient rings working again in Singular.jl

I would guess the first step for testing this is trying to get the command

Singular.PolynomialRing(Nemo.ZZ,["x"])

working, is that right?

@sebasguts sebasguts requested a review from wbhart December 1, 2018 12:58
@wbhart
Copy link
Contributor

wbhart commented Dec 1, 2018

Great! Yes, that would be a first step. Of course we have tests commented out for this.

@codecov
Copy link

codecov bot commented Dec 1, 2018

Codecov Report

Merging #73 into master will increase coverage by 7.22%.
The diff coverage is 65.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   46.33%   53.55%   +7.22%     
==========================================
  Files          30       31       +1     
  Lines        2398     2433      +35     
==========================================
+ Hits         1111     1303     +192     
+ Misses       1287     1130     -157
Impacted Files Coverage Δ
src/libsingular/LibSingularTypes.jl 100% <100%> (ø)
src/number/NumberTypes.jl 76.77% <100%> (+5.34%) ⬆️
src/libsingular/flint/fmpz.jl 60.65% <63.2%> (+60.65%) ⬆️
src/number/n_unknown.jl 31.25% <63.63%> (+31.25%) ⬆️
src/libsingular/coeffs.jl 50% <63.63%> (+14.7%) ⬆️
src/libsingular/nemo/Fields.jl 56.41% <69.38%> (+56.41%) ⬆️
src/poly/PolyTypes.jl 56.33% <80%> (+1.79%) ⬆️
src/poly/poly.jl 69.23% <0%> (+1.39%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7d433...f77ec29. Read the comment docs.

@wbhart
Copy link
Contributor

wbhart commented Dec 1, 2018

Looks good. Is the PR ready to go? Or should I wait for some test code to be switched on?

I can work on the other similar files, or I could get Sachin to work on it, depending on what you think.

@sebasguts
Copy link
Contributor Author

Not at all ready to go. It is just an initial setup. I will get it ready to go tomorrow and message you.

@sebasguts
Copy link
Contributor Author

@wbhart please have a look

@wbhart
Copy link
Contributor

wbhart commented Jan 4, 2019

Looks pretty neat. There are the following:

  • In NumberTypes.jl there is still a println
  • When creating the big struct, you can specify a bunch of zeroes in a row using [0 for i in 1:5]... and similarly for the Ptr{Void}'s. The ... syntax interpolates the array as a tuple. (Also is it necessary to defined the constructor for this type as an external constructor instead of an internal one?)
  • We should verify that the cache is still working correctly, e.g. by doing some computation that uses a lot of external coeffs and checking that the part that frees stuff from the cache ever gets triggered. The main thing that could go wrong is if actual addresses of the objects in question are not passed. So it presumably works, but always good to check, otherwise we will have a leak.

@sebasguts
Copy link
Contributor Author

I applied comments 1&2, but had to leave the outer constructor, as the ... syntax does not work inside new.

Do you have any test computation we could use? Or just run the tests a lot of times? I am pretty sure the caches work, though, as I had several problems including them.

@wbhart
Copy link
Contributor

wbhart commented Jan 4, 2019

It's not so much whether the caches work, but whether they still free the objects after they are no longer in use, or whether it just hangs on to them forever, essentially leaking memory.

I guess I don't have an example to try. Maybe the following will exercise it enough:

R, (x, y) = PolynomialRing(Nemo.ZZ, ["x", "y"])
f = x*y+2x+y+1
g = x*y^2+x^2*y
for i = 1:1000
   gcd(f^10*g^12, f^12*g^10)
end

See if memory usage grows without bound. Or better yet, put some print statements in the function that frees stuff from cache, to confirm stuff is actually being freed.

@wbhart wbhart mentioned this pull request Jun 19, 2019
42 tasks
@wbhart
Copy link
Contributor

wbhart commented Aug 21, 2019

I think I will now work on this and try to get it working again. We have someone at the Gap-Singular days that could really use this functionality.

@wbhart
Copy link
Contributor

wbhart commented Aug 26, 2019

Superceded.

@wbhart wbhart closed this Aug 26, 2019
fingolfin pushed a commit to fingolfin/Singular.jl that referenced this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants