Skip to content

Conversation

@rossviljoen
Copy link
Collaborator

No description provided.

@rossviljoen rossviljoen requested a review from ancapdev July 14, 2025 14:00
Copy link
Owner

@ancapdev ancapdev left a comment

Choose a reason for hiding this comment

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

Could you bump the version as well, and I'll merge and release

Makie.needs_tick_update_observable(conversion::UnixTimeConversion) = nothing

Makie.MakieCore.should_dim_convert(::Type{UnixTime}) = true
Makie.should_dim_convert(::Type{UnixTime}) = true
Copy link
Owner

Choose a reason for hiding this comment

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

Does this also work with 0.22?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it prints a warning but still works

@rossviljoen
Copy link
Collaborator Author

Could you bump the version as well, and I'll merge and release

Done

@ancapdev ancapdev merged commit 9e8a8bb into master Jul 14, 2025
@rossviljoen rossviljoen deleted the makie-updates branch July 15, 2025 16:34
@JoaoAparicio
Copy link
Contributor

Hey I don't think this works on Makie 0.24.

Here's what I tested.

using UnixTimes
import Makie
using CairoMakie

times = [
    UnixTime("2025-01-01T10:00:00.000000000"),
    UnixTime("2025-01-01T10:00:01.000000000"),
    UnixTime("2025-01-01T10:00:02.000000000"),
    UnixTime("2025-01-01T10:00:03.000000000"),
]

data = [1.0, 2.0, 1.0, 4.0]

Makie.lines(times, data)

Envs:

Works:

]add Makie@0.22 UnixTimes CairoMakie
⌃ [13f3f980] CairoMakie v0.13.10
⌅ [ee78f7c6] Makie v0.22.10
  [ab1a18e7] UnixTimes v1.7.1

Works:

]add Makie@0.23 UnixTimes CairoMakie
⌃ [13f3f980] CairoMakie v0.14.0
⌅ [ee78f7c6] Makie v0.23.0
  [ab1a18e7] UnixTimes v1.7.1

Doesn't work:

]add Makie@0.24.0 UnixTimes CairoMakie
⌃ [13f3f980] CairoMakie v0.15.0
⌅ [ee78f7c6] Makie v0.24.0
  [ab1a18e7] UnixTimes v1.7.1

The error is:

ERROR: ArgumentError: 
    Conversion failed for Lines (With conversion trait PointBased()) with args:
        Tuple{Vector{UnixTime}, Vector{Float64}} 
    Got converted to: Tuple{Vector{UnixTime}, Vector{Float64}}
    Lines requires to convert to argument types Tuple{AbstractVector{<:Union{Point2, Point3}}}, which convert_arguments didn't succeed in.
    To fix this overload convert_arguments(P, args...) for Lines or PointBased() and return an object of type Tuple{AbstractVector{<:Union{Point2, Point3}}}.`

Stacktrace:
 [1] argument_error(PTrait::PointBased, P::Type, args::Tuple{…}, user_kw::Dict{…}, converted::Tuple{…})
   @ Makie ~/.julia/packages/Makie/7m6za/src/compute-plots.jl:600
 [2] (Lines)(user_args::Tuple{Vector{UnixTime}, Vector{Float64}}, user_attributes::Dict{Symbol, Any})
   @ Makie ~/.julia/packages/Makie/7m6za/src/compute-plots.jl:648
 [3] _create_plot(::Function, ::Dict{Symbol, Any}, ::Vector{UnixTime}, ::Vararg{Any})
   @ Makie ~/.julia/packages/Makie/7m6za/src/figureplotting.jl:328
 [4] lines(::Vector{UnixTime}, ::Vararg{Any}; kw::@Kwargs{})
   @ Makie ~/.julia/packages/Makie/7m6za/src/recipes.jl:517
 [5] lines(::Vector{UnixTime}, ::Vararg{Any})
   @ Makie ~/.julia/packages/Makie/7m6za/src/recipes.jl:515
 [6] top-level scope
   @ ~/repos/Braid.jl/research/temp2.jl:14
Some type information was truncated. Use `show(err)` to see complete types.

Makie breaking release: https://github.com/MakieOrg/Makie.jl/releases/tag/v0.24.0

First line in the release notes is "Breaking Refactored plots to rely on the newly introduced ComputeGraph instead of Observables". However, Makie 0.24 still depends on Observables. So, I believe that they have moved on part of its internals from Observables to ComputeGraph, but not everything. Is this understanding is correct, maybe we should just hold off working on compat until Makie's internals are stabilized?

@rossviljoen rossviljoen mentioned this pull request Jul 20, 2025
@rossviljoen
Copy link
Collaborator Author

Quite right, I don't know how I missed that, it should be fixed by #16.

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.

4 participants