[Feat/aut1000] - SmartPeak support for exporting workflow results as plots #412

ahmedskhalil · 2021-08-27T14:26:21Z

Objectives

Exporting plots as PNG, PDF, HTML and SVG.

Sample plots

Original

As PDF (highest resolution)
smartpeak-exported-plot.pdf

As PNG

ghost · 2021-08-27T14:26:45Z

DeepCode's analysis on #31a75f found:

⚠️ 2 warnings 👇

Top issues

Description	Example fixes
Missing check is_open on std::ofstream before writing to it. Occurrences: Plotter.cpp:201 Plotter.cpp:203 Plotter.cpp:205	🔧 Example fixes
Leaking memory. PlotExporter is allocated on the heap and never freed. In function drawSliders. Occurrences: GraphicDataVizWidget.cpp:68	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

ghost · 2021-08-31T15:27:04Z

DeepCode's analysis on #4afb3e found:

⚠️ 1 warning 👇

Top issues

Description	Example fixes
Missing check is_open on std::ofstream before writing to it. Occurrences: Plotter.cpp:300 Plotter.cpp:313 Plotter.cpp:314	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

ghost · 2021-08-31T16:31:55Z

DeepCode's analysis on #0f2579 found:

⚠️ 1 warning 👇

Top issues

Description	Example fixes
Missing check is_open on std::ofstream before writing to it. Occurrences: Plotter.cpp:315 Plotter.cpp:316 Plotter.cpp:321	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

ghost · 2021-08-31T18:31:02Z

DeepCode's analysis on #086aad found:

⚠️ 1 warning 👇

Top issues

Description	Example fixes
Missing check is_open on std::ofstream before writing to it. Occurrences: Plotter.cpp:315 Plotter.cpp:316 Plotter.cpp:321	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

ghost · 2021-09-01T19:07:43Z

DeepCode's analysis on #b5d503 found:

⚠️ 1 warning 👇

Top issues

Description	Example fixes
Missing check is_open on std::ofstream before writing to it. Occurrences: Plotter.cpp:315 Plotter.cpp:316 Plotter.cpp:321	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

dmccloskey

Good idea using the GNU plotting library. Overall it looks quite nice to me. I look forward to seeing the demo tomorrow.

It would be good to have Bertrands review as well.

dmccloskey · 2021-09-08T11:10:40Z

src/smartpeak/source/ui/GraphicDataVizWidget.cpp

@@ -266,5 +279,33 @@ namespace SmartPeak
    }
    return result;
  }
+
+  void GraphicDataVizWidget::showInstallationInstructions()


dmccloskey · 2021-09-08T11:11:23Z

src/tests/class_tests/smartpeak/source/PlotExporter_test.cpp

+
+  std::filesystem::remove_all(main_path / "plots");
+  #endif
+}


Is the Win version missing?

I removed the directive to include Win

bertrandboudaud

It's nice, i would like to see a demo :)
I had some minor comments, but would be nice to handle them before to merge it.
one thing should be very nice is to use the FilePicker. to make the user choose where he want the plot to be saved.

bertrandboudaud · 2021-09-08T11:20:23Z

src/smartpeak/include/SmartPeak/ui/sources.cmake

@@ -18,6 +18,7 @@ set(sources_list_h
 	LogWidget.h
 	ParameterEditorWidget.h
 	ParametersTableWidget.h
+  Plotter.h


(minor) Here all lines are using tabs but you are using space. please use tab.

bertrandboudaud · 2021-09-08T11:23:58Z

src/smartpeak/source/ui/GraphicDataVizWidget.cpp

+    ImGui::SameLine();
+    if (ImGui::Button("Save Plot"))
+    {
+      PlotExporter* exported_plot = new PlotExporter(application_handler_.main_dir_.string(), graph_viz_data_, selected_format);


can't you use unique_ptr here ? (or just get a stack variable)

bertrandboudaud · 2021-09-08T11:24:45Z

src/smartpeak/source/ui/sources.cmake

@@ -17,6 +17,7 @@ set(sources_list
 	LogWidget.cpp
 	ParameterEditorWidget.cpp
 	ParametersTableWidget.cpp
+  Plotter.cpp


Please use tab instead of spaces here.

yes my editor had altered settings

bertrandboudaud · 2021-09-08T11:25:03Z

src/tests/class_tests/smartpeak/executables.cmake

@@ -30,6 +30,7 @@ set(core_executables_list
 set(io_executables_list
 	CSVWriter_test
 	ParametersParser_test
+  PlotExporter_test


Please use tab instead of spaces here.

bertrandboudaud · 2021-09-08T11:31:27Z

src/tests/class_tests/smartpeak/source/PlotExporter_test.cpp

+  std::filesystem::path main_path;
+  SmartPeak::SessionHandler::GraphVizData graph_viz_data_;
+
+  SmartPeak::Utilities::getEnvVariable("HOME", &home_path);


I would be more in favor to use std::filesystem::temp_directory_path to create a directory suitable for unit tests.

bertrandboudaud · 2021-09-08T11:32:49Z

src/tests/class_tests/smartpeak/source/PlotExporter_test.cpp

+
+  //PNG=0, PDF=1, HTML=2, SVG=3
+  #if defined(__APPLE__) || defined(__linux__)
+  SmartPeak::PlotExporter* exported_png = new SmartPeak::PlotExporter(main_path.string(), graph_viz_data_, 0);


It's always better to use smart pointer than the good old new/delete pair (or to construct on the stack?). is it possible?

bertrandboudaud · 2021-09-08T11:34:16Z

src/smartpeak/include/SmartPeak/ui/GraphicDataVizWidget.h

@@ -85,6 +90,7 @@ namespace SmartPeak
    std::optional<float> marker_position_;
    bool use_markers_ = false;
    bool is_spectra_ = false;
+//    const std::unique_ptr<std::filesystem::path> working_path_ = std::make_unique<std::filesystem::path>(application_handler_.main_dir_);


this could be remove ?

bertrandboudaud · 2021-09-08T11:38:33Z

src/smartpeak/source/ui/Plotter.cpp

+     }
+     return true;
+   } else {
+     LOGW << "gnuplot not found! Not generating any plots.";


It could be an error (LOGE).

bertrandboudaud · 2021-09-08T11:48:16Z

src/smartpeak/source/ui/GraphicDataVizWidget.cpp

@@ -22,6 +22,7 @@
 // --------------------------------------------------------------------------

 #include <SmartPeak/ui/GraphicDataVizWidget.h>
+#include <SmartPeak/ui/FilePicker.h>


I don't see you are using the FilePicker. it would be very nice to use it.

It wasn't implemented by the time of reviewing, now it's in.

[API,UI,FIX] - exporting plots as pdf, html, svg & png

31a75fd

ahmedskhalil linked an issue Aug 31, 2021 that may be closed by this pull request

SmartPeak crashes when given invalid directory #415

Closed

[API,DOC,TEST] - added docs

4afb3e5

[TEST] - minor changes

0f25790

minor fixes

086aad9

minor fix

b5d503f

ahmedskhalil added 6 commits September 3, 2021 10:57

[TEST] - minor changes

bc1a3b4

[TEST] - minor changes

3754f0f

[TEST] - minor changes

3830a46

[TEST] - minor changes

aa1513b

[CI] - packaging fix

8f88b98

[TEST] - minor changes

06aa6c2

ahmedskhalil requested review from dmccloskey and bertrandboudaud September 8, 2021 06:34

dmccloskey reviewed Sep 8, 2021

View reviewed changes

ahmedskhalil removed a link to an issue Sep 8, 2021

SmartPeak crashes when given invalid directory #415

Closed

bertrandboudaud requested changes Sep 8, 2021

View reviewed changes

ahmedskhalil added 8 commits September 8, 2021 18:50

[UI] - saving plots in user-defined locations

6c7894c

[TEST,CODE]- minor changes

6df6318

[TEST] - fix failing unittests on windows

55483a1

[TEST] - fix failing unittests on windows

8a052fc

[TEST] - minor changes

b1df413

[TEST] - fix for windows

59e5bf2

[API,TEST] - fixing test failures on Win. platforms

e8dd6b5

[API,TEST] - fixing test failures on Win. platforms

0b9753d

bertrandboudaud self-requested a review September 13, 2021 17:22

bertrandboudaud approved these changes Sep 13, 2021

View reviewed changes

bertrandboudaud merged commit 5f22eff into develop Sep 13, 2021

ahmedskhalil deleted the ft1000 branch January 7, 2022 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat/aut1000] - SmartPeak support for exporting workflow results as plots #412

[Feat/aut1000] - SmartPeak support for exporting workflow results as plots #412

ahmedskhalil commented Aug 27, 2021 •

edited

Loading

ghost commented Aug 27, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

ghost commented Sep 1, 2021

dmccloskey left a comment

dmccloskey Sep 8, 2021

dmccloskey Sep 8, 2021

ahmedskhalil Sep 9, 2021

bertrandboudaud left a comment

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

ahmedskhalil Sep 9, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

bertrandboudaud Sep 8, 2021

ahmedskhalil Sep 9, 2021

[Feat/aut1000] - SmartPeak support for exporting workflow results as plots #412

[Feat/aut1000] - SmartPeak support for exporting workflow results as plots #412

Conversation

ahmedskhalil commented Aug 27, 2021 • edited Loading

ghost commented Aug 27, 2021

DeepCode's analysis on #31a75f found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

ghost commented Aug 31, 2021

DeepCode's analysis on #4afb3e found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

ghost commented Aug 31, 2021

DeepCode's analysis on #0f2579 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

ghost commented Aug 31, 2021

DeepCode's analysis on #086aad found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

ghost commented Sep 1, 2021

DeepCode's analysis on #b5d503 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

dmccloskey 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

bertrandboudaud 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedskhalil commented Aug 27, 2021 •

edited

Loading