Introduce HeatSources #282

masterleinad · 2024-05-03T14:37:17Z

This is a step towards #190. This pull request replaces std::vector<std::shared_ptr<adamantine::HeatSource<dim>>> with HeatSources that contains all heat sources as Kokkos::Views and loops over them in its member functions. With this approach, there is no dynamic polymorphism and the compiler could inline all function calls for the heat sources. In particular, this object would be usable on GPUs (apart from function annotations).

stvdwtt · 2024-05-06T19:01:19Z

source/ScanPath.cc

@@ -138,7 +140,9 @@ void ScanPath::load_event_series_scan_path(std::string scan_path_file)
    segment.power_modifier = last_power;
    last_power = std::stod(split_line[4]);

-    _segment_list.push_back(segment);
+    if (_segment_list_length == MAX_NUMBER_OF_SEGMENTS)
+      Kokkos::abort("too many segmnets");


Typo:

Suggested change

Kokkos::abort("too many segmnets");

Kokkos::abort("too many segments");

stvdwtt · 2024-05-06T19:05:15Z

source/ScanPath.hh

@@ -19,6 +19,8 @@
 #include <istream>
 #include <vector>

+#define MAX_NUMBER_OF_SEGMENTS 100


This is too low -- if we had a 50-layer print with only two segments per layer, we'd hit this. I'm not sure that we've done enough sliced full parts to know a good upper bound. For example the AOP hourglass has 8055 segments and that's pretty simple as far as "real" parts go. Can we use a Kokkos::View with a runtime-defined length instead?

That was one of my next steps but I would need to manage the allocations for the segments outside the heat sources to avoid Views of managed Views. I'm not sure how much freedom we have to change those APIs.

The current version should be (more or less) device-usable already at least.

Ok, got it. Maybe another option would be to make it a template parameter? Even with the dynamic scan path in #223, we wouldn't be changing the length of the scan path too many times per simulation.

Sure, changing it to a template argument for the heat source and the scan path is easy. The question then is if we want to have a runtime dispatch for pre-instantiated maximum sizes or any other mechanism that doesn't require rebuilding the library and the application for certain input files.

We don't want to do this. We don't know how many segments we will need but it's potentially very large. If someone wants to do point-source raster for PBF, the number of segments will be huge.
In the current code, the scan path is part of the heat source. You need to change that. Move the scan path to HeatSources, add a function that evaluates the scan patch from t_0 to t_1 and fill a Kokkos::View. This way we don't need to know the number of segments and you don't waste memory if you have a ton a segments.

I'm not sure how much freedom we have to change those APIs.

adamantine is not a library. Everything but the input parameters is an implementation detail.

masterleinad · 2024-05-13T19:21:32Z

source/HeatSources.hh

+template <typename MemorySpace, int dim>
+std::vector<ScanPath<MemorySpace>>
+HeatSources<MemorySpace, dim>::get_scan_paths() const
+{
+  std::vector<ScanPath<MemorySpace>> scan_paths;
+  for (unsigned int i = 0; i < _electron_beam_heat_sources.size(); ++i)
+    scan_paths.push_back(_electron_beam_heat_sources(i).get_scan_path());
+  for (unsigned int i = 0; i < _goldak_heat_sources.size(); ++i)
+    scan_paths.push_back(_goldak_heat_sources(i).get_scan_path());
+  return scan_paths;
+}


Is it important for the order here to match the input file?

No it's not. There is no order in the input file. You can reorder everything and the same input file will work.

Rombur

You will need to rebase the PR on develop. There was some small changes in ScanPath.

Rombur · 2024-06-04T12:48:19Z

application/adamantine.hh

-        // coarsest mesh we may need to add other points to check (e.g.
-        // quadrature points, vertices).
-        for (unsigned int f = 0; f < cell->reference_cell().n_faces(); ++f)
+        if (heat_sources.max_value(cell->face(f)->center(), current_source_height) >


You should just use value. In practice, value and max_value will probably always be the same because heat sources do not overlap.

Rombur · 2024-06-04T12:51:52Z

application/adamantine.hh

@@ -609,14 +606,16 @@ void refine_mesh(
  const double refinement_beam_cutoff =
      refinement_database.get<double>("beam_cutoff", 1.0e-15);

+  adamantine::HeatSources<dim, dealii::MemorySpace::Host> host_heat_sources = heat_sources.copy_to(dealii::MemorySpace::Host{});


Please use heat_sources_host instead of host_heat_sources. We use the suffixes _host and _dev instead of the prefixes host_ and dev_. This helps when using autocomplete.

Rombur · 2024-06-04T12:52:30Z

application/adamantine.hh

@@ -936,11 +935,13 @@ run(MPI_Comm const &communicator, boost::property_tree::ptree const &database,
              mechanical_physics, displacement, material_properties, timers);
  ++n_time_step;

+  adamantine::HeatSources<dim, dealii::MemorySpace::Host> host_heat_sources = heat_sources.copy_to(dealii::MemorySpace::Host{});


Same comment

Rombur · 2024-06-04T12:53:12Z

source/CubeHeatSource.cc

@@ -9,11 +9,12 @@
 #include <instantiation.hh>
 #include <types.hh>

+#include <deal.II/base/memory_space.h>


Why did you add this include?

Rombur · 2024-06-04T12:54:03Z

source/CubeHeatSource.hh

@@ -17,7 +19,7 @@ namespace adamantine
 * used for verification purpose.
 */
 template <int dim>
-class CubeHeatSource final : public HeatSource<dim>
+class CubeHeatSource final


Suggested change

class CubeHeatSource final

class CubeHeatSource

Rombur · 2024-06-04T14:02:42Z

source/ThermalPhysics.templates.hh

-  }
-  _current_source_height = temp_height;
+  _current_source_height =
+      _heat_sources.copy_to(dealii::MemorySpace::Host{}).get_current_height(t);


Same comment as above

Rombur · 2024-06-04T14:05:47Z

source/ThermalPhysicsInterface.hh

@@ -19,9 +19,6 @@ namespace adamantine
 // Forward declarations
 class Timer;

-template <int dim>
-class HeatSource;
-


You don't need a forward declaration because you are relying on the order of the includes.

You still need a forward declaration, otherwise the code may break if the include are reordered.

Rombur · 2024-06-04T14:10:46Z

source/ScanPath.hh

+  }
+
+  static std::vector<ScanPathSegment>
+  extract_scan_paths(std::string scan_path_file, std::string file_format);


I don't really like this name. You are just reading from a file.

Rombur · 2024-06-04T14:15:53Z

tests/test_heat_source.cc

+  GoldakHeatSource<2, dealii::MemorySpace::Host> goldak_heat_source(
+      beam, ScanPath<dealii::MemorySpace::Host>(scan_paths_segments_view));
+  ElectronBeamHeatSource<2, dealii::MemorySpace::Host> eb_heat_source(
+      database, ScanPath<dealii::MemorySpace::Host>(scan_paths_segments_view));


Can you put this code in a function instead of copy/pasting three times.

Rombur · 2024-06-04T14:16:27Z

tests/test_material_deposition.cc

+      scan_paths_segments_view(scan_path_segments.data(),
+                               scan_path_segments.size());
+  adamantine::ScanPath<dealii::MemorySpace::Host> scan_path(
+      scan_paths_segments_view);


Put this code in a function to avoid the copy/paste.

…urces

masterleinad · 2024-06-13T17:42:22Z

Retest this please.

Rombur · 2024-06-18T14:41:30Z

Let me know when you want another review on this.

masterleinad · 2024-06-18T19:08:20Z

Let me know when you want another review on this.

This could take another review now. Some notes:

I create local copies on the host for the HeatSources object where necessary assuming that we want it by default to be in the memory space specified in the respective template argument for the other operators. It might make sense to keep it in host memory instead and only create device copies or we keep a member variable for both memory spaces (if they don't coincide).
I needed to move a bunch of const members in the heat sources to make these classes copyable. I was running into some issues with static variables if used on the device as well. Thus, I moved the respective constants into member functions.
The recent changes reusing scan file paths made the HeatSources code a little more complicated. We are now storing those std::strings in HeatSources directly as well and recreating the scan file segments from them if necessary. Since the ScanPath doesn't own the ScanPathSegments, reading the file is a static member instead of a member function named "extract_scan_paths". I'm happy to change it back to some other name like "read_file" again if preferred.
I believe I addressed all other comments.

Rombur · 2024-06-18T20:52:33Z

application/adamantine.hh

+            heat_sources_host =
+                heat_sources.copy_to(dealii::MemorySpace::Host{});
+        heat_sources_host.update_scan_paths();
+        heat_sources = heat_sources_host.copy_to(MemorySpaceType{});


I don't like that you are copying back and forth such a heavy class just to read a file. update_scan_path() should do the right thing.

Rombur · 2024-06-18T20:54:01Z

source/CubeHeatSource.cc

@@ -13,7 +13,6 @@ namespace adamantine
 {
 template <int dim>
 CubeHeatSource<dim>::CubeHeatSource(boost::property_tree::ptree const &database)
-    : HeatSource<dim>()


Update the copyright date to 2024

Rombur · 2024-06-18T20:54:23Z

source/CubeHeatSource.hh

@@ -8,7 +8,9 @@
 #ifndef CUBE_HEAT_SOURCE_HH
 #define CUBE_HEAT_SOURCE_HH

-#include <HeatSource.hh>
+#include <BeamHeatSourceProperties.hh>


This line should be removed and the copyright date updated.

Rombur · 2024-06-18T20:58:47Z

source/ElectronBeamHeatSource.cc

 {
+  static const double log_01 = std::log(0.1);


Rombur · 2024-06-18T21:00:13Z

source/ElectronBeamHeatSource.hh

+  void set_scan_path(ScanPath<MemorySpaceType> const scan_path)
+  {
+    _scan_path = scan_path;
+  }


Move the implementation out of the class declaration

Rombur · 2024-06-18T21:30:07Z

source/HeatSources.hh

+
+  for (unsigned int i = 0; i < _goldak_heat_sources.size(); ++i)
+    _goldak_heat_sources(i).set_scan_path(
+        ScanPath<MemorySpaceType>{_goldak_scan_path_segments[i]});


Why are we keeping track of the scan path in HeatSources and in each individual sources? We should not duplicate this information.

Rombur · 2024-06-18T21:32:34Z

source/HeatSources.hh

+    value += _cube_heat_sources(i).value(point, height);
+  for (unsigned int i = 0; i < _goldak_heat_sources.size(); ++i)
+    value += _goldak_heat_sources(i).value(point, height);
+  return value;


Rombur · 2024-06-18T21:38:20Z

source/ScanPath.cc

+
+template <typename MemorySpaceType>
+std::vector<ScanPathSegment>
+ScanPath<MemorySpaceType>::extract_scan_paths(std::string scan_path_file,


I still hate the name

Rombur · 2024-06-18T21:47:22Z

source/ScanPath.hh

@@ -54,6 +54,7 @@ class ScanPathTester;
 * gives the power modifier for the current segment. It reads in the scan path
 * from a text file.
 */
+template <typename MemorySpaceType>


Is is actually necessary to have the ScanPath on the GPU? I feel like this should only exists on the CPU and we should only move part of the data when it's necessary. Otherwise, we will end up using several GBs of memory on the GPU just for the ScanPath.

Rombur · 2024-06-18T21:49:39Z

source/ThermalPhysicsInterface.hh

@@ -19,9 +19,6 @@ namespace adamantine
 // Forward declarations
 class Timer;

-template <int dim>
-class HeatSource;
-


You still need a forward declaration, otherwise the code may break if the include are reordered.

…urces_work

stvdwtt reviewed May 6, 2024

View reviewed changes

masterleinad force-pushed the introduce_heat_sources branch from d1ce403 to 456fa4e Compare May 7, 2024 22:52

masterleinad added 4 commits May 10, 2024 16:36

Introduce HeatSources

8728e74

Update ScanPath implementation

7db8d9a

Store ScanPathSegments outside of ScanPath

9e31ea3

Refactor HeatSources to store std::vectors of ScanPathSegments

5fdff90

masterleinad force-pushed the introduce_heat_sources branch from 6d9d625 to ac4d8a9 Compare May 10, 2024 17:23

Remove HeatSource.hh

cf6263f

masterleinad force-pushed the introduce_heat_sources branch from ac4d8a9 to cf6263f Compare May 10, 2024 17:24

Copy to host where necessary and fix copy_to_host

4ec8fd0

masterleinad force-pushed the introduce_heat_sources branch from aaa3bdf to 4ec8fd0 Compare May 10, 2024 23:03

Restructure HeatSources

432c229

masterleinad commented May 13, 2024

View reviewed changes

masterleinad added 3 commits May 14, 2024 12:01

Add copy_to_device

9e520b7

Use correct heat source order in set_beam_properties

fce7f5c

Fix order of template parameters

9193862

masterleinad closed this May 16, 2024

masterleinad reopened this May 16, 2024

Fix copy_to when targeting GPUs

7b88824

masterleinad mentioned this pull request May 20, 2024

Avoid compiler warnings #287

Merged

masterleinad changed the title ~~[WIP] Introduce HeatSources~~ Introduce HeatSources May 20, 2024

masterleinad added 2 commits May 20, 2024 17:08

Remove KOKKOS_FUNCTION from HeatSources.hh

ac35083

Restore lac/la_vector.h

69394d1

masterleinad marked this pull request as ready for review May 20, 2024 21:28

Rombur requested changes Jun 4, 2024

View reviewed changes

masterleinad added 2 commits June 4, 2024 11:54

Merge remote-tracking branch 'upstream/master' into introduce_heat_so…

0025d0f

…urces

Introduce update_scan_paths

b467051

masterleinad force-pushed the introduce_heat_sources branch from a100e94 to b467051 Compare June 10, 2024 17:40

masterleinad added 2 commits June 10, 2024 16:07

Remove max_value

583ab35

host_heat_sources -> heat_sources_host

aef6051

masterleinad added 10 commits June 10, 2024 16:14

Indent more filles

3358d7f

Remove redundant include

647ee6f

Remove beam from cube heat source

76ff0bf

Remove redundant final

a788186

Resets -> Reset

87efdf7

Return BeamHeatSourceProperties by const&

dea04e4

Fix formatting

af4f181

_pi_over_3_to_1p5 -> pi_over_3_to_1p5

a95bcb3

Formatting

02a7600

Avoid duplications in tests

ab3a2d5

masterleinad force-pushed the introduce_heat_sources branch from 433f9ec to ab3a2d5 Compare June 11, 2024 17:16

Fix running on GPUs

24601c6

Rombur reviewed Jun 18, 2024

View reviewed changes

masterleinad added 4 commits June 25, 2024 14:46

extract_scan_paths->read_file

044ad9f

Store ScanPath on host

a01cbc5

Minimize changes

6f13c95

Merge remote-tracking branch 'upstream/master' into introduce_heat_so…

1f5f784

…urces_work

masterleinad force-pushed the introduce_heat_sources branch from 8c5c979 to 1f5f784 Compare July 24, 2024 17:18

masterleinad marked this pull request as draft July 24, 2024 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce HeatSources #282

Introduce HeatSources #282

masterleinad commented May 3, 2024 •

edited

Loading

stvdwtt May 6, 2024

stvdwtt May 6, 2024

masterleinad May 6, 2024

stvdwtt May 6, 2024

masterleinad May 6, 2024

Rombur May 6, 2024 •

edited

Loading

masterleinad May 13, 2024

Rombur May 14, 2024

Rombur left a comment

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 18, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

Rombur Jun 4, 2024

masterleinad commented Jun 13, 2024

Rombur commented Jun 18, 2024

masterleinad commented Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

Rombur Jun 18, 2024

	Kokkos::abort("too many segmnets");
	Kokkos::abort("too many segments");

Introduce HeatSources #282

Are you sure you want to change the base?

Introduce HeatSources #282

Conversation

masterleinad commented May 3, 2024 • 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

Rombur May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rombur 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

masterleinad commented Jun 13, 2024

Rombur commented Jun 18, 2024

masterleinad commented Jun 18, 2024

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

masterleinad commented May 3, 2024 •

edited

Loading

Rombur May 6, 2024 •

edited

Loading