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

float32 conversion #3663

Closed
wants to merge 26 commits into from
Closed

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Feb 29, 2024

As a PR against #3226, since its not working completely yet

TODO:

  • merge dont use macro for aliases and introduce more variants (Float64 -> d, UInt -> ui) JuliaGeometry/GeometryBasics.jl#214 to make shorthands available
  • adjust convert_arguments pipeline to either let numeric types pass through OR always convert them to Float32/Float64 as appropriate
  • update project, data_limits, boundingbox to handle Real inputs, make outputs Float64 or input based
  • update Axis limits to handle Real inputs, and use Float64 for limits
  • implement Float32Scaling type / scaling logic
  • implement matrix adjustments for scaling
  • Allow CairoMakie to work with Float64 instead of scaling
  • consider adding base + offset tick formatting (e.g. 1e9 + 1 .. 1e9 + 10)

Breaking Changes:

  • project functions may return Point{N, Float64} dependeing on their inputs
  • data_limits and boundingbox now return Rect3d instead of Rect3f
  • transform_func is now expected to keep the input type or convert to Float64
  • Axis limits are now Float64
  • BBox is overwritten to be a Rect3d. TODO This may clash with GridLayoutBase!

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 1, 2024

Keeping track of some thoughts/discussion.

On Float64 -> Float32 transform:

  • we need offset to treat float precision issues (i.e. difference in eps)
  • we need scaling to treat range issues (i.e. difference in floatmax)
  • it might be better to scale first, offset second, as scaling is safe
  • offset and scaling don't need to be exact, i.e. we don't need to end up in an exact range of values. We just need to be close enough to resolve the limit range adequately.
    • Float64 is enough to resolve up to Float64 and up to (U)Int32
    • eps(Float64(2^53-1)) = 1 is more or less the last (U)Int64 that Float64 can resolve

Other Notes:

  • internal projection (e.g. hlines) shouldn't cause issues since they transform to pixel space. I.e. we can either transform them with Float64 camera matrices and no float scaling or with float scaling and adjusted Float32 matrices

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 3, 2024

#3671 isn't strictly necessary for this pr, but I think it would be quite helpful. With stricter definitions of what data_limits and boundingbox do it's easier to reason about them and update them. That pr also should also be more or less done with converting these function to Float64.

I changed model to be a Mat4d but I don't think that' strictly necessary. Type promotion would make projection functions work regardless. On the other hand this allows scale! and translate! to use Float64 which might be useful?

For handling projection matrices with and without float32 conversion we had the following idea:

  • Generate Float64 matrices based on data with float32convert and patch them to be Float32-save in backends. For just Axis this would be fairly simple since view = I and projection is just scaling and translation. Generally this probably requires Standardize view and projection matrices #3550 to make view and projection consistent, and would be somewhat complicated as translations and scaling are usually split between view and projection.
  • Generate a patched Float32 and unpatched Float64 set of matrices. This moves the patching work to Makie, which is likely simpler since we don't need to disassemble and reassemble matrices.

Another option would be to always patch matrices in Makie (so keeping Float32) and handle float32 conversions as another matrix in CairoMakie. Since the float conversions are just scaling + translations they are easy to turn into a matrix and merge into the project functions. Assuming we project efficiently (i.e. multiply all matrices together once, not for each point) this should be practically free.

end

# For Vector{<: Real} applying to x/y/z dimension
function apply_transform_and_f32_conversion(
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we still may want a Float32Projection <: PointTransform type, and then create one of those per scene, and apply them directly in drawing_primitives.jl, like this

function cached_robj!(robj_func, screen, scene, plot::AbstractPlot)
    ...
    transform = Float32Transform(scene.f32convert) 
    gl_attributes[:transform] = transform 
    ....
    
end

function draw_atomic(screen::Screen, scene::Scene, ::Scatter)
    return cached_robj!(screen, scene, plot) do gl_attributes
        space = plot.space
        positions = handle_view(plot[1], gl_attributes)
        transform_func_obs = gl_attributes[:transform]
        # Leave all the transform func lifts, just change them coming from `plot` to `gl_attributes`:
        positions = lift(apply_transform, plot, transform_func_obs, positions, space)
        ...
    end
end

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that override the plot's native transform_func, though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We wouldn't want to replace existing transform_func's though, would we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I guess I meant:

transform = Float32Transform(scene.f32convert, plot.transform)) 

And then in apply_transform do the heavy lifting and apply the original transform...
We should not overwrite the plot.transform, that's why I put it in gl_attributes

@asinghvi17
Copy link
Member

Also, is this a good place to start looking at transforms for points/lines/etc? For example, if I want to convert all lines (but not polygons) to resampled geodesic paths...

else
# If we do any transformation, we have to assume things aren't on the grid anymore
# so x + y need to become matrices.
[apply_transform(t, Point(x, y), space) for x in x, y in y]
[MAkie.f32_convert(f32c, apply_transform(t, Point(x, y), space), space) for x in x, y in y]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[MAkie.f32_convert(f32c, apply_transform(t, Point(x, y), space), space) for x in x, y in y]
[Makie.f32_convert(f32c, apply_transform(t, Point(x, y), space), space) for x in x, y in y]

(typo fix)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, but I don't think it's worth reviewing details at this point. I'm currently prototyping without much care and plan to clean things up later, once we are more certain about whether this is a good direction to go.

Reviews on a more conceptual level would be more useful. For example if you see issues with how this would integrate with GeoMakie. I think I noted this somewhere before but the rough plan for the conversion pipeline here is:

  1. convert_arguments makes structural changes, e.g. Vector, Vector -> Vector{Point2}, leaving types alone or converting to Float32 and Float64 as appropriate
  2. apply transform_func, preferably keeping types
  3. apply Float32 conversion which may rescale data
  4. continue with model, camera as usual

I think Axis converts apply between 1 and 2, but I haven't paid much attention to that pr yet, tbh.

Also regarding transformed space - it's not the point of this pr to add it but I wouldn't mind. I want to add that and world space, but it's not a priority for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Axis converts apply between 1 and 2, but I haven't paid much attention to that pr yet, tbh.

No, axis convert shouldn't change anything about that at this point.

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 5, 2024

Also, is this a good place to start looking at transforms for points/lines/etc? For example, if I want to convert all lines (but not polygons) to resampled geodesic paths...

Does that mean you want/need transform_func(func, vector) rather than transform_func(func, element)?

@asinghvi17
Copy link
Member

asinghvi17 commented Mar 5, 2024

On second thought, this is a bit tricky. What I want is:

apply_transform(transform_func, plot_type, data::Vector)::(result::Vector)
# where
length(data) < length(result)

but that would require us to handle per-attribute values, like colors, as well, probably by interpolating.

I guess that this could be handled explicitly per atomic plot type?

What I want to be able to do is dynamically resample lines (but not any other point based plot, like scatter or text) in a geo axis. Maybe this goes in the axis conversion layer, then, but there would have to be a way to indicate that this particular transformation is nondimensional.

@SimonDanisch
Copy link
Member Author

Yeah this is not the right place to discuss this ;)

@SimonDanisch SimonDanisch changed the base branch from sd/axis-converts to breaking-0.21 March 6, 2024 13:37
@SimonDanisch SimonDanisch changed the base branch from breaking-0.21 to sd/axis-converts March 6, 2024 13:37
@SimonDanisch SimonDanisch mentioned this pull request Mar 6, 2024
* remove transformations from data_limits

* update plots

* update blocks

* do not consider child scenes in data_limits and boundingbox

* put out some fires

* note important changes

* rename function

* fix PolarAxis

* cleanup Text some more

* small fixes

* use apply_transform_and_model

* fix some bbox types

* fix typing

* fix typo

* undo zoom change & fix data-space text

* fix "Transforming lines"

* consider marker bboxes in meshscatter data_limits

* fix remaining test errors

* boundingbox -> text_boundingbox
@SimonDanisch
Copy link
Member Author

Closing in favor of #3681

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.

3 participants