Tick Marks layout #8

danielost · 2023-07-05T18:29:42Z

Worked on the issue:

Axis Tick Marks #4

The following PR contains the following features:

Major tick marks layout
Grid layout

This is what a plot looks like now:

The same plot, but with a grid:

Next steps:

Minor ticks layout
Colour picking
Font picking
Fixing label location (right now label position is not relative to the tick, so it breaks when a label is a big number). Will use Landmark to fix this
Using manual ticks (specified by the user)

noelwelsh

I think the code needs a bit of cleanup.

I also think having all these code within Plot will soon become unmanagable, but that is a refactoring that can be done later.

noelwelsh · 2023-07-06T19:27:15Z

core/shared/src/main/scala/chartreuse/Plot.scala

@@ -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


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.

noelwelsh · 2023-07-06T19:39:33Z

core/shared/src/main/scala/chartreuse/Plot.scala

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


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) }

noelwelsh · 2023-07-06T19:41:44Z

core/shared/src/main/scala/chartreuse/Plot.scala

+        xTicksSequence,
+        yTicksSequence
+      )
+      return withTitles(plotWithTicksAndAxesAndGrid)


Don't use return. Rewrite to an expression.

noelwelsh · 2023-07-06T20:04:39Z

core/shared/src/main/scala/chartreuse/Plot.scala

+    * 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(


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)

noelwelsh · 2023-07-06T20:15:32Z

core/shared/src/main/scala/chartreuse/Plot.scala

+        plot
+          .on(
+            OpenPath.empty
+              .moveTo(tick._1.x, yTicksMapped.min - 10)


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

noelwelsh · 2023-07-06T20:17:38Z

core/shared/src/main/scala/chartreuse/Plot.scala

+      .toList
+  }
+
+  /** Converts `Ticks` to a list of tuples. The first element is the mapped


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.

noelwelsh

A few more changes here, I think.

noelwelsh · 2023-07-10T10:59:02Z

core/shared/src/main/scala/chartreuse/Plot.scala

+
+  def draw(width: Int, height: Int): PlotPicture = {
+    val dataBoundingBox = layers.foldLeft(BoundingBox.empty) { (bb, layer) =>
+      bb.on(layer.data.boundingBox(layers.head.toPoint))


layers.head.toPoint is wrong. Different layers could have different toPoint functions. This only compiles due to the overly restrictive type of layers (should probably be Layer[?, Alg] not Layer[A, Alg])

Suggested change

bb.on(layer.data.boundingBox(layers.head.toPoint))

bb.on(layer.data.boundingBox(layer.toPoint))

It might be worth adding a boundingBox method to Layer.

noelwelsh · 2023-07-10T11:20:19Z

core/shared/src/main/scala/chartreuse/Coordinate.scala

+
+import doodle.core.Point
+
+object Coordinate {


This wrapper object is not needed, if I'm correctly reading the documentation on opaque types.

noelwelsh · 2023-07-10T11:20:49Z

core/shared/src/main/scala/chartreuse/Plot.scala

@@ -20,20 +20,24 @@ import chartreuse.layout.ScatterPlot
 import doodle.algebra.*
 import doodle.core.*
 import doodle.syntax.all.*
+import chartreuse.Coordinate._


Suggested change

import chartreuse.Coordinate._

import chartreuse.Coordinate.*

_ is Scala 2 syntax and will be removed in the future.

noelwelsh · 2023-07-10T11:38:38Z

core/shared/src/main/scala/chartreuse/Plot.scala

+              .path
+          )
+          .on(
+            text(((dataCoordinate.x * 1000).round / 1000.0).toString)


Might look at https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/text/NumberFormat.html instead of this rather ad-hoc method.

noelwelsh · 2023-07-10T11:54:23Z

core/shared/src/main/scala/chartreuse/Plot.scala

+        plot
+          .on(
+            OpenPath.empty
+              .moveTo(xTicksMapped.min - 10, screenCoordinate.y)


Ad-hoc magic numbers need explanation. I assume this is a temporary decision that will be replaced by style options later.

noelwelsh · 2023-07-10T11:54:46Z

core/shared/src/main/scala/chartreuse/Plot.scala

+      )
+  }
+
+  private def withYTicks(


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.

noelwelsh · 2023-07-10T11:56:00Z

core/shared/src/main/scala/chartreuse/Plot.scala

+      plot: PlotPicture,
+      yTicksMapped: Ticks
+  ): PlotPicture = {
+    xTicksSequence


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

noelwelsh · 2023-07-10T11:59:00Z

core/shared/src/main/scala/chartreuse/Plot.scala

+    val plotWithTicksAndAxes =
+      withAxes(plotWithXAndYTicks, xTicksMapped, yTicksMapped)
+
+    if (grid) {


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.

danielost · 2023-07-11T08:45:09Z

build task fails because of NumberFormat.

Same issue is described here:
https://stackoverflow.com/questions/40165940/referring-to-a-non-existent-class-java-io-file

We probably need to find a replacement for NumberFormat.

danielost · 2023-07-11T16:47:10Z

I think it is not necessary to add the whole Plot implementation for JS and JVM and it must be fixed. But I just want to make sure everything else is OK.

noelwelsh

LGTM!

danielost · 2023-07-12T07:07:47Z

Thanks!

danielost added 11 commits June 25, 2023 15:57

Add data min and max evaluation to Plot

f1b69e4

Merge branch 'main' of https://github.com/danielost/chartreuse

e6f86e7

Merge branch 'creativescala:main' into main

6f46d75

Add tick marks layout

39625b5

Add major ticks layout

6bcc79f

Add grid layout

760efa2

Replace interpolatingSpline with OpenPath and ClosedPath

d16691c

Refactor Plot

ef71598

Add TicksSequence type to Plot

c3c79b8

Add comments to Plot

5fef71d

Add return types to plot methods

12b42b3

noelwelsh requested changes Jul 6, 2023

View reviewed changes

danielost added the enhancement New feature or request label Jul 7, 2023

danielost linked an issue Jul 7, 2023 that may be closed by this pull request

Add plot abstraction #1

Closed

danielost added 7 commits July 7, 2023 13:44

Fix bounding box calculation in Plot

f2ca421

Replace return with an expression

bff47ef

Remove duplicate code in Plot

18ec3a5

Refactor tuple member access

634ceb0

Move Alg constraint to draw

08963ff

Update documentation for ticksToSequence

2c7c02c

Add opaque types for screen and data coordinates

f8adeb4

danielost requested a review from noelwelsh July 8, 2023 18:38

danielost added 2 commits July 9, 2023 14:19

Remove duplicate extension from Coordinate

a8c2778

Fix formatting

689f21d

noelwelsh requested changes Jul 10, 2023

View reviewed changes

danielost added 5 commits July 10, 2023 18:19

Fix mistake in bounding box calculation

a82130d

Remove wrapper object for opaque types

c4b4c1a

Add boundingBox method to Layer

06110c4

Refactor builder methods in Plot

e3626d9

Replace magic numbers with named values

937453a

danielost added 2 commits July 11, 2023 10:49

Use NumberFormat instead of manual rounding

679ac97

Remove A type parameter from Plot

eb731e5

danielost added 2 commits July 11, 2023 12:08

Update PlotExample

cb73dd3

Use Intl.NumberFormat instead of java.text.NumberFormat for Scala.js

7132d28

Fix typos in NumberFormat

482d74e

noelwelsh approved these changes Jul 11, 2023

View reviewed changes

danielost merged commit a8d5bcd into creativescala:main Jul 12, 2023

danielost mentioned this pull request Jul 12, 2023

Outline the structure of the documentation #11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tick Marks layout #8

Tick Marks layout #8

danielost commented Jul 5, 2023

noelwelsh left a comment

noelwelsh Jul 6, 2023 •

edited

Loading

noelwelsh Jul 6, 2023

noelwelsh Jul 6, 2023

noelwelsh Jul 6, 2023

noelwelsh Jul 6, 2023

noelwelsh Jul 6, 2023

noelwelsh left a comment

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

noelwelsh Jul 10, 2023

danielost commented Jul 11, 2023

danielost commented Jul 11, 2023

noelwelsh left a comment

danielost commented Jul 12, 2023

	bb.on(layer.data.boundingBox(layers.head.toPoint))
	bb.on(layer.data.boundingBox(layer.toPoint))

	import chartreuse.Coordinate._
	import chartreuse.Coordinate.*

Tick Marks layout #8

Tick Marks layout #8

Conversation

danielost commented Jul 5, 2023

Worked on the issue:

The following PR contains the following features:

noelwelsh left a comment

Choose a reason for hiding this comment

noelwelsh Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelwelsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielost commented Jul 11, 2023

danielost commented Jul 11, 2023

noelwelsh left a comment

Choose a reason for hiding this comment

danielost commented Jul 12, 2023

noelwelsh Jul 6, 2023 •

edited

Loading