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.

Sign up for GitHub

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

Tick Marks layout #8

Merged
merged 30 commits into from
Jul 12, 2023
Merged

Tick Marks layout #8

Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
f1b69e4
Add data min and max evaluation to `Plot`
danielost Jun 25, 2023
e6f86e7
Merge branch 'main' of https://github.com/danielost/chartreuse
danielost Jun 29, 2023
6f46d75
Merge branch 'creativescala:main' into main
danielost Jul 2, 2023
39625b5
Add tick marks layout
danielost Jul 3, 2023
6bcc79f
Add major ticks layout
danielost Jul 3, 2023
760efa2
Add grid layout
danielost Jul 4, 2023
d16691c
Replace `interpolatingSpline` with `OpenPath` and `ClosedPath`
danielost Jul 4, 2023
ef71598
Refactor `Plot`
danielost Jul 5, 2023
c3c79b8
Add `TicksSequence` type to `Plot`
danielost Jul 5, 2023
5fef71d
Add comments to `Plot`
danielost Jul 5, 2023
12b42b3
Add return types to plot methods
danielost Jul 6, 2023
f2ca421
Fix bounding box calculation in `Plot`
danielost Jul 7, 2023
bff47ef
Replace `return` with an expression
danielost Jul 7, 2023
18ec3a5
Remove duplicate code in `Plot`
danielost Jul 7, 2023
634ceb0
Refactor tuple member access
danielost Jul 7, 2023
08963ff
Move `Alg` constraint to `draw`
danielost Jul 8, 2023
2c7c02c
Update documentation for `ticksToSequence`
danielost Jul 8, 2023
f8adeb4
Add opaque types for screen and data coordinates
danielost Jul 8, 2023
a8c2778
Remove duplicate extension from `Coordinate`
danielost Jul 9, 2023
689f21d
Fix formatting
danielost Jul 9, 2023
a82130d
Fix mistake in bounding box calculation
danielost Jul 10, 2023
c4b4c1a
Remove wrapper object for opaque types
danielost Jul 10, 2023
06110c4
Add `boundingBox` method to `Layer`
danielost Jul 10, 2023
e3626d9
Refactor builder methods in `Plot`
danielost Jul 11, 2023
937453a
Replace magic numbers with named values
danielost Jul 11, 2023
679ac97
Use `NumberFormat` instead of manual rounding
danielost Jul 11, 2023
eb731e5
Remove `A` type parameter from `Plot`
danielost Jul 11, 2023
cb73dd3
Update `PlotExample`
danielost Jul 11, 2023
7132d28
Use `Intl.NumberFormat` instead of `java.text.NumberFormat` for Scala.js
danielost Jul 11, 2023
482d74e
Fix typos in `NumberFormat`
danielost Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 192 additions & 11 deletions core/shared/src/main/scala/chartreuse/Plot.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ import doodle.syntax.all.*
*/
final case class Plot[
A,
Alg <: Layout & Shape
Alg <: Layout & Shape & Style & Text & doodle.algebra.Transform & Path
Copy link
Contributor

@noelwelsh noelwelsh Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is incorrect. This is a constraint on layers, and this seems an overly restrictive to require that they implement these algebras. (I'm not sure why the original constraint was Layout & Shape. I don't think it needs to be anything other than doodle.algebra.Algebra.) This constraint should be on draw, I think.

](
layers: List[Layer[A, Alg]],
plotTitle: String = "Plot Title",
xTitle: String = "X data",
yTitle: String = "Y data",
grid: Boolean = true
grid: Boolean = false
) {
type TicksSequence = Seq[(Point, Point)]

def addLayer(layer: Layer[A, Alg]): Plot[A, Alg] = {
copy(layers = layer :: layers)
}
Expand All @@ -50,19 +52,197 @@ final case class Plot[
copy(yTitle = newYTitle)
}

// TODO: handle axes and grid layout
def draw(width: Int, height: Int): Picture[
Alg & Text & doodle.algebra.Transform & Debug,
Unit
] = {
val plot =
def withGrid(newGrid: Boolean): Plot[A, Alg] = {
copy(grid = newGrid)
}

def draw(width: Int, height: Int): Picture[Alg, Unit] = {
val allData = layers.flatMap(_.data.foldLeft(List.empty[A])(_ :+ _))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only to calculate the enclosing bounding box, right? It's quite inefficient to do all this copying.

I think this will do:

layers.foldLeft(BoundingBox.empty){ (accum, layer) => 
  accum.on(layer.data.boundingBox)
}

val dataBoundingBox = Data(allData).boundingBox(layers.head.toPoint)

val minX = dataBoundingBox.left
val maxX = dataBoundingBox.right
val minY = dataBoundingBox.bottom
val maxY = dataBoundingBox.top

val scale = Scale.linear.build(dataBoundingBox, width, height)

val xTicks = TickMarkCalculator.calculateTickScale(minX, maxX, 12)
val yTicks = TickMarkCalculator.calculateTickScale(minY, maxY, 12)

// Map the Ticks to the screen coordinates
val xTicksMapped = Ticks(
scale(Point(xTicks.min, 0)).x,
scale(Point(xTicks.max, 0)).x,
xTicks.size
)
val yTicksMapped = Ticks(
scale(Point(0, yTicks.min)).y,
scale(Point(0, yTicks.max)).y,
yTicks.size
)

// Convert the Ticks to a sequence of points
val xTicksSequence = xTicksToSequence(xTicks, scale)
val yTicksSequence = yTicksToSequence(yTicks, scale)

val allLayers =
layers
.map(_.draw(width, height))
.foldLeft(empty[Alg])(_ on _)
.margin(20)
.debug(color = Color.black)
.margin(5)

val plotWithXTicks = withXTicks(xTicksSequence, allLayers, yTicksMapped)
val plotWithXAndYTicks =
withYTicks(yTicksSequence, plotWithXTicks, xTicksMapped)
val plotWithTicksAndAxes =
withAxes(plotWithXAndYTicks, xTicksMapped, yTicksMapped)

if (grid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If all the with methods (withGrid, withXTicks`) just returned the picture instead of adding it to the plot, you could write something like

xTicks
  .on(yTicks)
  .on(axes)
  .on(allLayers)
  .on(if grid then theGrid else empty[Shape])

I.e. emphasize composition.

val plotWithTicksAndAxesAndGrid = withGrid(
plotWithTicksAndAxes,
xTicksMapped,
yTicksMapped,
xTicksSequence,
yTicksSequence
)
return withTitles(plotWithTicksAndAxesAndGrid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use return. Rewrite to an expression.

}

withTitles(plotWithTicksAndAxes)
}

/** Converts `Ticks` to a list of tuples. The first element is the mapped
* coordinate of a tick - to place the tick on a graph. The second one is the
* original coordinate - to give the tick a label with its coordinate.
*/
private def xTicksToSequence(
Copy link
Contributor

Choose a reason for hiding this comment

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

Too much repetition in this code. There shouldn't be repeated code for the x- and y-axis. You can abstract over x- and y-coordinates using a function. E.g.

val asX: Double => Point = x => Point(x, 0)
val asY: Double => Point = y => Point(0, y)

xTicks: Ticks,
scale: Bijection[Point, Point]
): TicksSequence = {
(0 to ((xTicks.max - xTicks.min) / xTicks.size).toInt)
.map(i =>
(
scale(Point(xTicks.min + i * xTicks.size, 0)),
Point(xTicks.min + i * xTicks.size, 0)
)
)
.toList
}

/** Converts `Ticks` to a list of tuples. The first element is the mapped
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be clearer on what these coordinates mean. I think of screen vs data coordinates.

Screen coordinates are the coordinates of the graph rendered on the screen. Data coordinates are the values in the data.

Consider using opaque types to distinguish these at the type level, as I think it will be very easy to get them confused in code.

* coordinate of a tick - to place the tick on a graph. The second one is the
* original coordinate - to give the tick a label with its coordinate.
*/
private def yTicksToSequence(
yTicks: Ticks,
scale: Bijection[Point, Point]
): TicksSequence = {
(0 to ((yTicks.max - yTicks.min) / yTicks.size).toInt)
.map(i =>
(
scale(Point(0, yTicks.min + i * yTicks.size)),
Point(0, yTicks.min + i * yTicks.size)
)
)
.toList
}

private def withXTicks(
xTicksSequence: TicksSequence,
plot: Picture[Alg, Unit],
yTicksMapped: Ticks
): Picture[Alg, Unit] = {
xTicksSequence
Copy link
Contributor

Choose a reason for hiding this comment

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

Repetition of this code and withYTicks is somewhat annoying. It would be nice to remove it.

.foldLeft(plot)((plot, tick) =>
plot
.on(
OpenPath.empty
.moveTo(tick._1.x, yTicksMapped.min - 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit not-fan of using these methods on tuple. I'd rather destructure with an irrefutable match like so:

val (screen, _) = tick

.lineTo(tick._1.x, yTicksMapped.min - 17)
.path
)
.on(
text(((tick._2.x * 1000).round / 1000.0).toString)
.at(tick._1.x, yTicksMapped.min - 30)
)
)
}

private def withYTicks(
Copy link
Contributor

Choose a reason for hiding this comment

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

If this method only returned the PlotPicture that represents the ticks, instead of adding it to the plot, there would be more possibilities for composition in the use above.

yTicksSequence: TicksSequence,
plot: Picture[Alg, Unit],
xTicksMapped: Ticks
): Picture[Alg, Unit] = {
yTicksSequence
.foldLeft(plot)((plot, tick) =>
plot
.on(
OpenPath.empty
.moveTo(xTicksMapped.min - 10, tick._1.y)
.lineTo(xTicksMapped.min - 17, tick._1.y)
.path
)
.on(
text(((tick._2.y * 1000).round / 1000.0).toString)
.at(xTicksMapped.min - 45, tick._1.y)
)
)
}

private def withAxes(
plot: Picture[Alg, Unit],
xTicksMapped: Ticks,
yTicksMapped: Ticks
): Picture[Alg, Unit] = {
plot
.on(
ClosedPath.empty
.moveTo(xTicksMapped.min - 10, yTicksMapped.min - 10)
.lineTo(xTicksMapped.max + 10, yTicksMapped.min - 10)
.lineTo(xTicksMapped.max + 10, yTicksMapped.max + 10)
.lineTo(xTicksMapped.min - 10, yTicksMapped.max + 10)
.path
)
}

private def withGrid(
plot: Picture[Alg, Unit],
xTicksMapped: Ticks,
yTicksMapped: Ticks,
xTicksSequence: TicksSequence,
yTicksSequence: TicksSequence
): Picture[Alg, Unit] = {
plot.on(
xTicksSequence
.foldLeft(empty[Alg])((plot, tick) =>
plot
.on(
OpenPath.empty
.moveTo(tick._1.x, yTicksMapped.min - 20)
.lineTo(tick._1.x, yTicksMapped.max + 10)
.path
.strokeColor(Color.gray)
.strokeWidth(0.5)
)
)
.on(
yTicksSequence
.foldLeft(empty[Alg])((plot, tick) =>
plot
.on(
OpenPath.empty
.moveTo(xTicksMapped.min - 10, tick._1.y)
.lineTo(xTicksMapped.max + 10, tick._1.y)
.path
.strokeColor(Color.gray)
.strokeWidth(0.5)
)
)
)
)
}

private def withTitles(plot: Picture[Alg, Unit]): Picture[Alg, Unit] = {
val plotTitle = text(this.plotTitle)
.scale(2, 2)
val xTitle = text(this.xTitle)
Expand All @@ -72,6 +252,7 @@ final case class Plot[
yTitle
.beside(
plot
.margin(5)
.below(plotTitle)
.above(xTitle)
)
Expand Down