-
Notifications
You must be signed in to change notification settings - Fork 130
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
Eagon northcott complex #4327
base: master
Are you sure you want to change the base?
Eagon northcott complex #4327
Conversation
Tests are failing, but I can not reproduce a single one of these failures. Is any of these problems known? Ping @benlorenz @aaruni96 |
3ead720
to
51ae766
Compare
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4327 +/- ##
==========================================
- Coverage 84.52% 84.33% -0.19%
==========================================
Files 646 656 +10
Lines 85823 86875 +1052
==========================================
+ Hits 72539 73268 +729
- Misses 13284 13607 +323
|
Sorry, but I still do not understand what's going on here. I can not reproduce the error. You are talking about 1.6 of what? Oscar? Julia? Hecke? Nemo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to improve the coverage a bit
Attention: Patch coverage is
53.81356%
with218 lines
in your changes missing coverage. Please review.
especially src/Modules/Iterators.jl where just 8% of the new code is tested.
experimental/DoubleAndHyperComplexes/src/Objects/eagon_northcott_complex.jl
Outdated
Show resolved
Hide resolved
…tt_complex.jl Co-authored-by: Benjamin Lorenz <[email protected]>
Yes. I was thinking of putting the easier examples from my paper here as test files. That way people also have the possibility to reproduce the computations. Is this a legitimate approach? @JHanselman ? |
That would of course help. However, please then add a comment somewhere that these are more like regression tests aka you are not certain that the current result is correct, but you want failures if the result changes. And (if possible) please add some basic tests that can be either verified by hand or using other software (although this may be hard to do/impossible) |
One file with very low patch coverage is |
@wdecker seems interested, and will touch by next week (likely this week) with @HechtiDerLachs to discuss more. |
@HechtiDerLachs I am pretty sure @thofma referred to Julia 1.6 (which we currently still support). After all he was referring to language feature: "keys for generators (comprehensions) is not available on 1.6". In addition, none of Oscar, Hecke, Nemo have reached version 1.6... |
@benlorenz gave a more detailed explanation what's going on below and I could already do similar fixes on other PRs myself after that. So this question has been clarified. But thank you for your explanation.
|
There are plenty of tests added which are disabled, which I have mixed feelings about. If this has to do with having the code for some paper be available somewhere, I wonder whether uncommented tests in a specific Oscar commit is the right place for this. (This also "violates" the Mardi (FAIR?) principle, since it is not reproducible.) I don't think this should block this PR here, but it might be better to discuss this specific issue in-person in case this comes up again. |
Thanks for the feedback. I also do not find this solution really viable at the moment and will eventually move on to something else. I will consult with the appropriate people in person to learn about better approaches. |
This is the branch with the full functionality for what's used in this preprint. I would like to bring these things to OSCAR, but piece by piece. The first round was in #4248 , so once that one is merged, I will rebase this PR again. For the moment, I would just like to work on the tests and polish this here a bit.Ready for review. This introduces