Skip to content

Refactor code base#163

Merged
pkofod merged 1 commit intoJuliaNLSolvers:masterfrom
devmotion:dmw/refactor
Nov 11, 2025
Merged

Refactor code base#163
pkofod merged 1 commit intoJuliaNLSolvers:masterfrom
devmotion:dmw/refactor

Conversation

@devmotion
Copy link
Contributor

@devmotion devmotion commented Nov 10, 2025

This PR

  • replaces == nothing with === nothing
  • replaces typeof(x) <: T with x isa T (IIRC better for the compiler)
  • replaces some broadcasting with fill!
  • reduces allocations in a few places
  • reduces construction of strings in show method
  • uses lazy strings for error messages
  • defines Base.eltype only for type but not instance
  • removes unnecessary __precompile__ statement
  • fixes a bug in TwiceDifferentiableConstraints
  • updates CI

Fixes #151.

@devmotion devmotion force-pushed the dmw/refactor branch 6 times, most recently from 2f8210b to 83d532b Compare November 10, 2025 20:13
@devmotion devmotion marked this pull request as ready for review November 10, 2025 20:19
@devmotion devmotion force-pushed the dmw/refactor branch 6 times, most recently from aac2a2b to 3b30f75 Compare November 10, 2025 23:19
ccache_righttype = zeros(promote_type(T, eltype(_x)), nc)
c!(ccache_righttype, _x)
return sum(_λ[i] * ccache[i] for i in eachindex(_λ, ccache))
return LinearAlgebra.dot(_λ, ccache_righttype)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkofod this seems to be a bug that could explain spurious IPNewton test failures.

Copy link
Member

Choose a reason for hiding this comment

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

good catch

function parse_constraints(::Type{T}, l, u) where T
size(l) == size(u) || throw(DimensionMismatch("l and u must be the same size, got $(size(l)) and $(size(u))"))
if size(l) != size(u)
throw(DimensionMismatch(LazyString("Size of lower bounds (", size(l), ") and number of upper bounds (", size(u), ") must be equal.")))
Copy link
Member

Choose a reason for hiding this comment

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

I see you typically use lazy but LazyString here. Any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the implememtation of the macro in base Julia 😅 And noticed all the parsing and string indexing going on there. This caused me to reconsider my (and our) use of the macro - it seems a bit unnecessary to hide all this information by using the macro if you can directly provide it with the constructor, even though it only affects the compilation times.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, it's meant as an optimization so why settle :)

@pkofod
Copy link
Member

pkofod commented Nov 11, 2025

This all looks reasonable. I think we can get rid of appveyor.

@pkofod pkofod merged commit 950a12c into JuliaNLSolvers:master Nov 11, 2025
9 of 10 checks passed
@devmotion devmotion deleted the dmw/refactor branch November 11, 2025 09:22
@devmotion
Copy link
Contributor Author

Could we tag a new releases? #164 would be breaking, so it might be good to tag a non-breaking release with the improvements before merging it. I already updated the version number.

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.

Why aref_calls, df_calls and h_calls all Vector{Int}?

2 participants