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

Fixes #24: representation extensions; provides a plotting extension: scatterplots, line plots, decision boundaries etc. #47

Merged
merged 25 commits into from
Sep 30, 2024

Conversation

lukstafi
Copy link
Contributor

There's no interpolation for line plots, and no nice graphics in the HTML backend -- it simply follows the text backend by placing boxes at the right coordinates.

@lukstafi
Copy link
Contributor Author

All should be finished and tested now, ready for review.

@lukstafi
Copy link
Contributor Author

It's simple to make printbox-html a depopts of printbox-ext-plot, but adds two extra files. Let me know if that's worthwhile.

@lukstafi
Copy link
Contributor Author

I just fixed HTML overlap ordering (z-index) and made framed=opaque. Now the the overlap of nested boxes output on HTML is as almost as good as in text -- only missing crop-to-canvas, and misplaced endpoints of axis labels (which is outside the scope of the plotting extension, it's printbox-html 's fault).

image

Copy link
Owner

@c-cube c-cube left a comment

Choose a reason for hiding this comment

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

I think this is a great feature, and the plotting part is excellent.

I have a concern or two about the extensibility story. type ext = .. is good; however I think the other types and tables are not at the right place.

Ideally, each backend (text, html, etc.) would have its own extension handlers table (e.g. (string, (ext -> html)) hashtbl or (string, (ext -> PrintBox_text.B.t)) Hashtbl.t). Then each extension (e.g plotting), extends ext, and registers a corresponding extension handler to PrintBox_text and PrintBox_html's respective tables.

What do you think?

@lukstafi
Copy link
Contributor Author

Alright, it will be simpler indeed.

@lukstafi
Copy link
Contributor Author

Done!

Copy link
Owner

@c-cube c-cube left a comment

Choose a reason for hiding this comment

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

Very nice, I only have minor nitpicks now. Thank you very much!

src/printbox-ext-plot/PrintBox_ext_plot.mli Show resolved Hide resolved
src/printbox-ext-plot/PrintBox_ext_plot.mli Outdated Show resolved Hide resolved
test/dune Show resolved Hide resolved
@c-cube c-cube merged commit 540e3db into c-cube:main Sep 30, 2024
3 checks passed
@c-cube
Copy link
Owner

c-cube commented Sep 30, 2024

Thank you!!

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.

2 participants