Deprecate lead, trail, lm, lt, lc, valence and coeffs. #805

wbhart · 2021-03-17T09:57:42Z

I have made PRs against all the other repos.

wbhart · 2021-03-17T10:01:33Z

Fixes #802
Fixes #779
Fixes #508

codecov · 2021-03-17T10:07:11Z

Codecov Report

Merging #805 (d2a5d43) into master (cbb00e1) will increase coverage by 0.02%.
The diff coverage is 79.80%.

@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
+ Coverage   81.05%   81.08%   +0.02%     
==========================================
  Files          52       52              
  Lines       17797    17792       -5     
==========================================
  Hits        14426    14426              
+ Misses       3371     3366       -5

Impacted Files	Coverage Δ
src/AbstractAlgebra.jl	`47.92% <ø> (ø)`
src/Deprecations.jl	`0.00% <ø> (ø)`
src/Generic.jl	`100.00% <ø> (ø)`
src/algorithms/MPolyFactor.jl	`33.86% <0.00%> (ø)`
src/generic/Misc/Poly.jl	`0.00% <0.00%> (ø)`
src/generic/SparsePoly.jl	`26.84% <81.25%> (-0.18%)`	⬇️
src/generic/Poly.jl	`91.87% <82.25%> (+<0.01%)`	⬆️
src/generic/MPoly.jl	`91.19% <83.33%> (ø)`
src/algorithms/LaurentPoly.jl	`96.59% <100.00%> (ø)`
src/generic/Matrix.jl	`91.42% <100.00%> (+0.15%)`	⬆️
... and 4 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 cbb00e1...d2a5d43. Read the comment docs.

wbhart · 2021-03-17T14:46:15Z

The OscarCI tests are exiting with signal 11 with no stack trace. Any idea what that is about?

thofma · 2021-03-17T14:55:35Z

Very weird, looking at the timestamps does not help either:

Wed, 17 Mar 2021 10:19:17 GMT SLPoly        |  414    414
Wed, 17 Mar 2021 10:19:17 GMT┌ Warning: `lead(f)` is deprecated, use `leading_coefficient(f)` instead.
Wed, 17 Mar 2021 10:19:17 GMT│   caller = leading_coefficient(f::fmpq_poly) at Poly.jl:127
Wed, 17 Mar 2021 10:19:17 GMT└ @ Hecke ~/work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Hecke/src/Misc/Poly.jl:127
Wed, 17 Mar 2021 13:13:07 GMT ERROR: Package Oscar errored during testing (received signal: 11)
Wed, 17 Mar 2021 13:13:07 GMT Stacktrace:

Let's restart.

wbhart · 2021-03-17T15:09:23Z

There's a lot of "unsatisfiable requirements" (dependencies) as well. I don't know if that caused it somehow.

wbhart · 2021-03-17T17:18:51Z

I have decided to do map_coeffs in a separate series of PR's if we decide to rename that. This is getting too messy otherwise. Thus, I will finish this off as soon as 801 is merged.

thofma · 2021-03-19T09:02:41Z

#801 is merged.

How do we want to move forward? We can merge all the breaking things (+ the to be created PR for map_coeff) today or beginning of next week and immediately tag a new release.

fieker · 2021-03-19T09:05:22Z

On Fri, Mar 19, 2021 at 02:03:00AM -0700, thofma wrote: #801 is merged. How do we want to move forward? We can merge all the breaking things (+ the to be created PR for map_coeff) today or beginning of next week and immediately tag a new release.

I'd rather do this earlier than later. There will be more of the same I am sure, but we canot wait and let the PRs get stale. I can lend a hand if you tell me what to do (only one exam today)

…

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #805 (comment)

wbhart · 2021-03-19T09:39:24Z

Please do not merge anything else until I have reviewed it. The order of merging here is potentially going to create some massive headaches.

I am on it now and will let you know when everything is ok to go.

wbhart · 2021-03-19T10:16:45Z

@thofma I believe this is right to go, independently of everything else. But I have not tested that the deprecations in AA are sufficient for Hecke/Oscar, etc. I'll test Nemo and Hecke now, and if so will call it good. Just give me 30 mins or so to run the tests.

wbhart · 2021-03-19T10:18:54Z

Current failure is just #788

wbhart · 2021-03-19T10:21:24Z

Nemo is fine, lots of yellow warnings, which is what I wanted to see. But tests pass.

Waiting for Hecke tests now.

wbhart · 2021-03-19T10:32:33Z

Hmm, the Hecke tests just seem to have hung without running a single test. 10 mins and counting. Does it do that sometimes?

wbhart · 2021-03-19T10:33:26Z

It seemed to be hung in deprecated.jl

thofma · 2021-03-19T10:37:08Z

Weird. Let me also try it locally.

wbhart · 2021-03-19T10:37:38Z

Looks like the same problem on restart of the tests. It displays:

[ Info: long_test: false
[ Info: short_test: false
┌ Warning: `lead(f)` is deprecated, use `leading_coefficient(f)` instead.
│   caller = leading_coefficient(::AbstractAlgebra.Generic.Poly{nf_elem}) at Poly.jl:127
└ @ Hecke ~/.julia/dev/Hecke/src/Misc/Poly.jl:127

Then it just hangs. Any idea what is happening here? It seems like the deprecations don't work with Hecke.

wbhart · 2021-03-19T10:42:04Z

Here's the trace when I hit CTRL-C:

^CERROR: InterruptException:
Stacktrace:
 [1] poptask(::Number fields: Error During Test at Base.InvasiveLinkedListSynchronized{Task}) at ./task.jl:704
 [2] wait at ./task.jl:712 [inlined]
 [3] wait(::Base.GenericCondition{Base.Threads.SpinLock}) at ./condition.jl:106
 [4] wait(::Base.Process) at ./process.jl:621
 [5] success at ./process.jl:483 [inlined]
 [6] /home/wbhart/.julia/dev/Hecke/test/NumField.jl:1
  Got exception outside of a @test
  LoadError: InterruptException:
  Stacktrace:run
(   [1] backtrace() at ./error.jl:112::
Cmd   [2] macro expansion at ./deprecated.jl:89 [inlined];
   [3] macro expansion at ./logging.jl:321 [inlined]
   [4] depwarn(::String, ::Symbol; force::Bool) at ./deprecated.jl:85
   [5] depwarn(::String, ::Symbol) at ./deprecated.jl:80
   [6] lead(::AbstractAlgebra.Generic.Poly{nf_elem}) at ./deprecated.jl:71
   [7] leading_coefficient(::AbstractAlgebra.Generic.Poly{nf_elem}) at /home/wbhart/.julia/dev/Hecke/src/Misc/Poly.jl:127
   [8] lead(::AbstractAlgebra.Generic.Poly{nf_elem}) at ./deprecated.jl:72
   ... (the last 2 lines are repeated 9389 more times)
   [18787] leading_coefficient(::AbstractAlgebra.Generic.Poly{nf_elem}) at /home/wbhart/.julia/dev/Hecke/src/Misc/Poly.jl:127
   [18788] _preproc_pol(::AbstractAlgebra.Generic.Poly{nf_elem}, ::AbstractAlgebra.Generic.Poly{nf_elem}) at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Poly.jl:142
   [18789] gcd_modular_kronnecker(::AbstractAlgebra.Generic.Poly{nf_elem}, ::AbstractAlgebra.Generic.Poly{nf_elem}, ::Bool) at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Poly.jl:179
   [18790] gcd at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Poly.jl:74 [inlined]
   [18791] issquarefree at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Poly.jl:432 [inlined]
   [18792] isirreducible_easy(::AbstractAlgebra.Generic.Poly{nf_elem}) at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Elem.jl:547
   [18793] isirreducible at /home/wbhart/.julia/dev/Hecke/src/NumField/NfAbs/Elem.jl:532 [inlined]
   [18794] #NumberField#1340 at /home/wbhart/.julia/dev/Hecke/src/NumField/NfRel/NfRel.jl:177 [inlined]
   [18795] #NumberField#1341 at /home/wbhart/.julia/dev/Hecke/src/NumField/NfRel/NfRel.jl:183 [inlined]
   [18796] NumberField(::AbstractAlgebra.Generic.Poly{nf_elem}) at /home/wbhart/.julia/dev/Hecke/src/NumField/NfRel/NfRel.jl:183
   [18797] top-level scope at /home/wbhart/.julia/dev/Hecke/test/NumField/Elem.jl:7
   [18798] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
   [18799] top-level scope at /home/wbhart/.julia/dev/Hecke/test/NumField/Elem.jl:2
   [18800] include(::String) at ./client.jl:457
   [18801] top-level scope at /home/wbhart/.julia/dev/Hecke/test/NumField.jl:2
   [18802] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1115
   [18803] top-level scope at /home/wbhart/.julia/dev/Hecke/test/NumField.jl:2
   [18804] include(::String) at ./client.jl:457
   [18805] top-level scope at timing.jl:174
   [18806] top-level scope at /home/wbhart/.julia/dev/Hecke/test/runtests.jl:78
   [18807] include(::String) at ./client.jl:457
   [18808] top-level scope at none:6
   [18809] eval(::Module, ::Any) at ./boot.jl:331
   [18810] exec_options(::Base.JLOptions) at ./client.jl:272
  in expression starting at /home/wbhart/.julia/dev/Hecke/test/NumField/Elem.jl:1

thofma · 2021-03-19T10:51:03Z

Yeah, I just saw it locally. There is a recursion:

       [6] lead(f::AbstractAlgebra.Generic.Poly{nf_elem})
         @ AbstractAlgebra ./deprecated.jl:71
       [7] leading_coefficient(f::AbstractAlgebra.Generic.Poly{nf_elem})
         @ Hecke ~/.julia/packages/Hecke/qJVuE/src/Misc/Poly.jl:127
       [8] lead(f::AbstractAlgebra.Generic.Poly{nf_elem})
         @ AbstractAlgebra ./deprecated.jl:72

I think we can merge as is.

wbhart · 2021-03-19T10:52:56Z

Maybe. I'm trying the Hecke tests with this, the Nemo and Hecke patches all dev'd (so there are no deprecations activated).

Let me check that works first.

wbhart · 2021-03-19T10:54:35Z

The issue is likely that Hecke currently defines leading_coefficient to be lead.

thofma · 2021-03-19T10:58:04Z

I removed the method in Hecke locally and I am now running the tests.

wbhart · 2021-03-19T11:03:48Z

Naturally my Hecke PR already removes this method that is causing the recursion.

wbhart · 2021-03-19T11:16:09Z

Remarkably Hecke passes without any issues with this, the Nemo and Hecke PRs that I made.

I'm surprised about that, but you can't help good luck!

thofma · 2021-03-19T11:18:00Z

Yes, very nice. Feel free to merge #803, #805, #806 in whatever order is the easiest.

wbhart · 2021-03-19T11:20:27Z

Nemo also. We should buy a lottery ticket!

So if you want to go ahead and review and merge this PR, you are welcome to.

Then I will rebase my multivariates on it while you review and merge #806
assuming it doesn't also need a rebase.

src/AbstractAlgebra.jl

wbhart · 2021-03-19T11:47:21Z

Thanks. I haven't set my VS Code editor up correctly yet. So it still uses tabs equal to 4 spaces and uses actual tabs not spaces. I just didn't find time for that yet. Still so many other problems with my new laptop. :-(

wbhart · 2021-03-19T13:31:35Z

Are all the Oscar CI runs strictly necessary? It looks like there's still about 3 hours of CI before we get an answer on this one!

Can't we cut the Oscar CI runs down to a bare minimum?

thofma · 2021-03-19T13:52:09Z

At the moment the backlog is for the normal "Run tests", which are just the usual AA tests, see https://github.com/Nemocas/AbstractAlgebra.jl/actions. The problem is that it runs the tests on every commit, but does not cancel the old runs. Do you know if one can make it cancel the old runs with GitHub Actions @benlorenz?

thofma · 2021-03-19T14:45:45Z

Oh boy, it created a lot of combinations to check. We have to tweak this a bit I think.

wbhart · 2021-03-19T14:47:25Z

Ja

thofma · 2021-03-19T14:49:48Z

I can't tweak it right now. You have tested it locally. Should we just merge?

wbhart · 2021-03-19T14:54:42Z

I'd say so. I've only tested it as far as Nemo and Hecke. I can't test it further in any easy manner right now. In particular I do not guarantee that the patches against Singular and Oscar are correct. They should be carefully reviewed before merging them.

wbhart · 2021-03-19T14:55:34Z

Technically I didn't test against the changes you made that I merged. But if you are confident of no typos...

wbhart · 2021-03-19T14:56:55Z

They were just whitespace anyhow, assuming you didn't delete a comma by accident

wbhart · 2021-03-19T15:04:04Z

I think it is not currently as useful as it could be. For example, it might be useful to know if Singular#deprecate_coeffs, Nemo#deprecate_coeffs, AA#deprecate_coeffs passes, but not with Oscar#master,Hecke#master if there are also deprecate_coeffs PRs for those packages.

Somehow I really want to know where in the chain things broke.

wbhart · 2021-03-19T15:06:15Z

Also I guess now that we are maintaining master against master it is not likely that master against releases is going to pass due to package manager things.

thofma · 2021-03-19T15:08:25Z

Yeah, it should probably disable the other tests if it finds a branch in a downstream package. I don't know how easy this is. I will mention it to Benjamin.

wbhart mentioned this pull request Mar 17, 2021

Change lead->leading_coefficient (tail, lm, lt, lc, valence and coeffs do not occur) Nemocas/Nemo.jl#1023

Closed

This was referenced Mar 17, 2021

Deprecate lead, trail, lt, lm, lc, valence, coeffs thofma/Hecke.jl#217

Closed

Deprecate coeffs oscar-system/Singular.jl#393

Merged

wbhart mentioned this pull request Mar 18, 2021

Deprecate coeffs oscar-system/Oscar.jl#359

Merged

wbhart added 3 commits March 19, 2021 10:58

Deprecate lead, trail, lm, lt, lc, valence and coeffs.

d762da9

Remove Misc functions that have been moved (rebase).

fe00a47

Replace lead -> leading_coefficient in moved Misc functions.

d2a5d43

wbhart force-pushed the deprecate_coeffs branch from e7b10d7 to d2a5d43 Compare March 19, 2021 10:07

wbhart changed the title ~~[WIP] Deprecate lead, trail, lm, lt, lc, valence and coeffs.~~ Deprecate lead, trail, lm, lt, lc, valence and coeffs. Mar 19, 2021

wbhart mentioned this pull request Mar 19, 2021

Multivariate series #803

Merged

thofma approved these changes Mar 19, 2021

View reviewed changes

src/AbstractAlgebra.jl Show resolved Hide resolved

src/AbstractAlgebra.jl Show resolved Hide resolved

src/AbstractAlgebra.jl Show resolved Hide resolved

src/AbstractAlgebra.jl Show resolved Hide resolved

wbhart force-pushed the deprecate_coeffs branch from 0bb17bd to d2a5d43 Compare March 19, 2021 11:57

thofma merged commit dc595c6 into Nemocas:master Mar 19, 2021

thofma mentioned this pull request Mar 20, 2021

Cut down number of jobs in the presence of feature branches oscar-system/OscarDevTools.jl#9

Closed

Deprecate lead, trail, lm, lt, lc, valence and coeffs. #805

Deprecate lead, trail, lm, lt, lc, valence and coeffs. #805

Conversation

wbhart commented Mar 17, 2021 • edited Loading

wbhart commented Mar 17, 2021

codecov bot commented Mar 17, 2021 • edited Loading

Codecov Report

wbhart commented Mar 17, 2021

thofma commented Mar 17, 2021

wbhart commented Mar 17, 2021

wbhart commented Mar 17, 2021

thofma commented Mar 19, 2021

fieker commented Mar 19, 2021 via email

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021 • edited Loading

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021 • edited Loading

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

wbhart commented Mar 19, 2021

thofma commented Mar 19, 2021

wbhart commented Mar 17, 2021 •

edited

Loading

codecov bot commented Mar 17, 2021 •

edited

Loading

wbhart commented Mar 19, 2021 •

edited

Loading

thofma commented Mar 19, 2021 •

edited

Loading