Skip to main content
edited body
Source Link
user73941
user73941

You use a named tuple as data type for points. Why not use the buildbuilt in PointF instead. Or - if you want the double precision in calculations - make your own PointD data type - and then abstract the conversion to and from Point and PointF away in cast operators. I think that would make the entire code more readable. There are a lot of inline casting, that could be avoided this way, and you could use the Graphics.DrawXXX(...) overrides that take PointF arguments instead of x,y arguments.

You use a named tuple as data type for points. Why not use the build in PointF instead. Or - if you want the double precision in calculations - make your own PointD data type - and then abstract the conversion to and from Point and PointF away in cast operators. I think that would make the entire code more readable. There are a lot of inline casting, that could be avoided this way, and you could use the Graphics.DrawXXX(...) overrides that take PointF arguments instead of x,y arguments.

You use a named tuple as data type for points. Why not use the built in PointF instead. Or - if you want the double precision in calculations - make your own PointD data type - and then abstract the conversion to and from Point and PointF away in cast operators. I think that would make the entire code more readable. There are a lot of inline casting, that could be avoided this way, and you could use the Graphics.DrawXXX(...) overrides that take PointF arguments instead of x,y arguments.

added 666 characters in body
Source Link
user73941
user73941
      for (var y = 0.0; y <= 1.0; y += 0.1)
      {
        var x1 = scale(0.0, 0.0, 1.0, graph_start_x, graph_end_x);
        var x2 = scale(1.0, 0.0, 1.0, graph_start_x, graph_end_x);
        ...

In some of the loops you instantiate x1, x2 or y1, y2 inside the loop although they are invariant while the loop is running. Consider to declare them before the loop:

      var x1 = scale(0.0, 0.0, 1.0, graph_start_x, graph_end_x); are constant => out of loop
      var x2 = scale(1.0, 0.0, 1.0, graph_start_x, graph_end_x);
      for (var y = 0.0; y <= 1.0; y += 0.1)
      {
        ...

      for (var y = 0.0; y <= 1.0; y += 0.1)
      {
        var x1 = scale(0.0, 0.0, 1.0, graph_start_x, graph_end_x);
        var x2 = scale(1.0, 0.0, 1.0, graph_start_x, graph_end_x);
        ...

In some of the loops you instantiate x1, x2 or y1, y2 inside the loop although they are invariant while the loop is running. Consider to declare them before the loop:

      var x1 = scale(0.0, 0.0, 1.0, graph_start_x, graph_end_x); are constant => out of loop
      var x2 = scale(1.0, 0.0, 1.0, graph_start_x, graph_end_x);
      for (var y = 0.0; y <= 1.0; y += 0.1)
      {
        ...

Source Link
user73941
user73941

When drawing graphics I admit that it in general can be difficult to split the code into meaningful sub methods because often many variables are used across different sections of the process. A way to do it could be to make a non static class, that holds the entire data set, constant key measurements and other resources as member fields/properties and then split the huge method plot_curve(...) into meaningful sub methods.

At least I would extract the preprocessing of the input data and the postprocessing of the resulting bmp in to dedicated methods.


List<(double x, double y)

You use a named tuple as data type for points. Why not use the build in PointF instead. Or - if you want the double precision in calculations - make your own PointD data type - and then abstract the conversion to and from Point and PointF away in cast operators. I think that would make the entire code more readable. There are a lot of inline casting, that could be avoided this way, and you could use the Graphics.DrawXXX(...) overrides that take PointF arguments instead of x,y arguments.


  if (perf_curve_type == 0)
  {
    throw new ArgumentException("Invalid performance curve type specified.", nameof(perf_curve_type));
  }

You make this test, but what about values above 4? Should it even be necessary for an enum type?


  List<(string x_title, string y_title, List<(double x, double y)> xy)> parsed_xy = encoded_plot_data.Split(new char[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries)
      .Select(line =>
      {
        var headers = line.Trim().Split('/').First().Split(';');
        var r = (x_title: headers[0], y_title: headers[1], xy: line.Trim().Split('/').Skip(1).Select(b => (x: double.Parse("" + b.Split(';')[0]), y: double.Parse("" + b.Split(';')[1]))).ToList());
        r.xy = r.xy.OrderBy(a => a.x).ThenBy(a => perf_curve_type == perf_curve_types.roc ? a.y : 1 - a.y).ToList();
        return r;
      }).ToList();

This fails for the provided test input, because there is a whitespace line as the last line. You need to filter that out before the Select(line...) statement:

  CultureInfo parseFormat = CultureInfo.InvariantCulture;
  List<(string x_title, string y_title, List<(double x, double y)> xy)> parsed_xy = encoded_plot_data.Split(new char[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries)
    .Where(line => !string.IsNullOrWhiteSpace(line)) 
    .Select(line =>
    {
      var headers = line.Trim().Split('/').First().Split(';');

      var r = (x_title: headers[0], y_title: headers[1], xy: line.Trim().Split('/').Skip(1).Select(b => (x: double.Parse("" + b.Split(';')[0], parseFormat), y: double.Parse("" + b.Split(';')[1], parseFormat))).ToList());

      r.xy = r.xy.OrderBy(a => a.x).ThenBy(a => perf_curve_type == perf_curve_types.roc ? a.y : 1 - a.y).ToList();
      return r;
    }).ToList();

As shown above you also need a proper format provider, when parsing the coordinates and other numbers in order to handle '.' and ',' correctly in respect to the file format.

Have you considered to use a Regex pattern instead?


.ToList()

You have a lot of these calls where you make a LINQ queries. For large data sets they might be expensive, because they create a copy of the data set. They should be unnecessary when just querying the set, so consider to avoid them as much as possible and only use them when you want to create a sub set reused more than once.

For instance:

public static double area_under_curve_trapz(List<(double x, double y)> coordinate_list)
{
  coordinate_list = coordinate_list.Distinct().ToList();
  coordinate_list = coordinate_list.OrderBy(a => a.x).ThenBy(a => a.y).ToList();
  var auc = coordinate_list.Select((c, i) => i >= coordinate_list.Count - 1 ? 0 : (coordinate_list[i + 1].x - coordinate_list[i].x) * ((coordinate_list[i].y + coordinate_list[i + 1].y) / 2)).Sum();
  return auc;
}

could be written as:

public static double area_under_curve_trapz(List<(double x, double y)> coordinate_list)
{
  var auc = coordinate_list
    .Distinct()
    .OrderBy(a => a.x)
    .ThenBy(a => a.y)
    .Select((c, i) => 
      i >= coordinate_list.Count - 1 
      ? 0 
      : (coordinate_list[i + 1].x - coordinate_list[i].x) * ((coordinate_list[i].y + coordinate_list[i + 1].y) / 2)).Sum();
  return auc;
}

It's just a query, that doesn't modify the source set, so it's safe to operate on that directly.


In general all graphic resources like pens, fonts, brushes etc as:

new SolidBrush(title_color)

should be disposed after use, so wrap them in a using statement.


new Font("Tahoma", (int)graph_start_y / 4, GraphicsUnit.Pixel) new Font("Tahoma", (int)graph_start_x / 4, GraphicsUnit.Pixel);

You repeatedly recreate these fonts, but graph_start_x and graph_start_y are constant throughout the method, so create them once


All in all I think the output is quite nice. I would make some adjustments in font sizes and the placement of the AUC results, and I wouldn't rotate the values on the x-axis.

Consider to use a bitmap format that is compressed - for instance png. The produced output in bmp format has a size of about 15 MB where a png only takes < 1 MB.