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

Big refresh, slightly breaking #11

Merged
merged 9 commits into from
May 22, 2024
Merged

Big refresh, slightly breaking #11

merged 9 commits into from
May 22, 2024

Conversation

gdalle
Copy link
Collaborator

@gdalle gdalle commented May 17, 2024

Hey there @michel2323!

This PR fixes #9 by modernizing and cleaning up the package, in the hope of getting it the appreciation it deserves. I tried to apply the current best practices in terms of package structure, as described in my blog https://modernjuliaworkflows.github.io/. I also added a lot of documentation, and a few breaking changes which I feel make sense (see below). However, the basic functionalities were not altered.

These changes affect ADNLPModels.jl (ping @amontoison) but the fixes are easy to apply so I'll take care of them if you accept.

Source code

  • BREAKING: Remove AbstractColoring, AbstractOrdering and the lists COLORINGS and ORDERINGS
  • BRERAKING: Replace them by single structs ColoringMethod and ColoringOrder with one string attribute (instead of an abstract type)
  • BREAKING: Implement d1_coloring and friends as shortcuts to these structs (instead of identical but separate structs). They no longer accept a string argument, since it was redundant.
  • BREAKING: Change the ColPackColoring to use references instead of length-1 vectors
  • Replace ccall with the macro @ccall
  • Structure into subfiles

Strictly speaking, since nothing was documented, these changes could be considered non-breaking, because in Julia the docs is the API ;)

Documentation

  • Set up a Documenter.jl pipeline for a proper docs website
  • Add tutorial page
  • Add API reference page
  • Write docstrings for everything (please double check that I understood the code correctly)
  • Clean up README

Tests

  • Fix the source of randomness for the sparse matrix with StableRNGs.jl (otherwise it depends on the value of Julia)
  • Adjust the number of colors we expect with each coloring
  • Add formal test with Aqua.jl and JET.jl
  • Add doctests with Documenter.jl
  • Structure into subfiles
  • Run Julia 1.6 in CI

Example

Esthetics

  • Add .JuliaFormatter.toml file for uniform style enforcement, apply formatting with JuliaFormatter.jl

Compat

  • Bump version to v0.4.0
  • Add compat bounds on Julia stdlibs
  • Only test with Aqua.jl and JET.jl on Julia v1.10 and above

CI

  • Add dependabot.yml to track outdated dependencies
  • Modernize Test.yml and TagBot.yml
  • Add Documentation.yml to build a docs website and deploy it

@michel2323
Copy link
Member

michel2323 commented May 17, 2024

LGTM (besides the missing new line in the yml files). As per the discussion with @amontoison I'd move the package to the Exanauts org if you don't mind. I added you to this repository so you can push the branch and the CI runs.

@gdalle
Copy link
Collaborator Author

gdalle commented May 17, 2024

Cool, thanks! I'll finish the PR tomorrow. I'm also writing a pure Julia version so we'll be able to compare speed 😊

@amontoison
Copy link
Member

@gdalle Can you update the Julia wrappers to use the macro @ccall?

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@gdalle
Copy link
Collaborator Author

gdalle commented May 18, 2024

LGTM (besides the missing new line in the yml files).

To have passing tests I had to fix the source of randomness generating the sparse matrix (otherwise the results you get depend on the version of Julia). I also had to modify the number of colors accordingly for some coloring algorithms. @michel2323 can you check that it still looks okay? Then I think we're good to merge.

@gdalle Can you update the Julia wrappers to use the macro @ccall?

@amontoison it's done but I've never written a line of C so please double-check

@gdalle gdalle mentioned this pull request May 18, 2024
src/colpackcoloring.jl Outdated Show resolved Hide resolved
src/colpackcoloring.jl Outdated Show resolved Hide resolved
src/colpackcoloring.jl Outdated Show resolved Hide resolved
src/colpackcoloring.jl Outdated Show resolved Hide resolved
src/colpackcoloring.jl Outdated Show resolved Hide resolved
src/colpackcoloring.jl Outdated Show resolved Hide resolved
gdalle and others added 2 commits May 18, 2024 10:12
@gdalle gdalle requested a review from michel2323 May 18, 2024 08:59
@gdalle
Copy link
Collaborator Author

gdalle commented May 22, 2024

@michel2323 should we merge?

@michel2323 michel2323 merged commit 75119bf into exanauts:master May 22, 2024
3 checks passed
@gdalle gdalle deleted the gd/refresh branch May 22, 2024 16:56
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.

Refresh package
4 participants