Add PointCloud spatial distribution #3161

gonuke · 2024-10-05T13:56:03Z

Description

Allows users to define a list of points in space, each with a different relative intensity, to be sampled discretely. This is a valid SpatialDistribution and can be used anywhere that a SpatialDistribution is valid.

Fixes #3159

Checklist

I have performed a self-review of my own code
I have run clang-format (version 15) on any C++ source files (if applicable)
I have followed the style guidelines for Python source files (if applicable)
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

…tings

gonuke · 2024-10-05T22:17:46Z

include/openmc/distribution_spatial.h

+class PointCloud : public SpatialDistribution {
+public:
+  explicit PointCloud(pugi::xml_node node);
+  explicit PointCloud(gsl::span<const double> x, gsl::span<const double> y,


Note: this could just as easily be a vector of Positions but I wasn't sure how to do that in XML space, so decided to just keep it this way for now. Happy for any guidance on changing this to a different form.

Could also overload the constructor with a few choices, but not sure of OpenMC design practices.

I think ideally we'd add the points as a vector of Position's. Best approach for this would be to add a function for parsing a set of positions from an XML to the xml_interface.h.

It looks like adding an input stream operator for Position will allow existing code to work, although not sure about robustness

include/openmc/distribution_spatial.h

gonuke · 2024-10-05T22:19:49Z

openmc/stats/multivariate.py

+        if isinstance(given_positions, list):
+            cv.check_length('first position entry', given_positions[0], 3, 3)
+            self._positions = np.asarray(given_positions)
+        elif isinstance(given_positions, np.ndarray):
+            self._positions = given_positions


Another place to seek OpenMC design best practice about being tolerant to lots of data forms for this data

The trend seems to be storing NumPy arrays for the internal attributes values when in doubt. I'll let @paulromano chime in here though in case his perspective differs.

gonuke · 2024-10-05T22:20:48Z

openmc/stats/multivariate.py

+        for idx, axis in enumerate(('x','y','z')):
+            subelement = ET.SubElement(element, axis)
+            subelement.text = ' '.join(str(e) for e in self.positions[idx,:])


assuming there is no better way to store a list of points in XML

Could imagine flattening this to a single list of [[x], [y], [z]] or a single list of [ (x,y,z) for zip(x,y,z)], but not sure it's a better choice

gonuke · 2024-10-05T22:23:28Z

include/openmc/distribution_spatial.h

+  double total_strength() { return this->point_idx_dist_.integral(); }
+
+private:
+  gsl::span<const double> x_, y_, z_;


could assemble these into a vector of Position on initialization, but not sure pros/cons of that choice vs. keeping these separate...

To me it makes sense to use vector<Position> to reduce multiple indexing operations that may be prone to error if changes are made down the line.

One more thought, we should make sure to use a container here so the object owns the memory of these attributes -- gsl::span is just a view into memory space and doesn't "own" these values.

pshriwise

Thanks for this snappy PR @gonuke! A useful distribution that we haven't been able to support before!

pshriwise · 2024-10-08T15:59:48Z

include/openmc/distribution_spatial.h

+class PointCloud : public SpatialDistribution {
+public:
+  explicit PointCloud(pugi::xml_node node);
+  explicit PointCloud(gsl::span<const double> x, gsl::span<const double> y,


I think ideally we'd add the points as a vector of Position's. Best approach for this would be to add a function for parsing a set of positions from an XML to the xml_interface.h.

pshriwise · 2024-10-09T01:58:56Z

include/openmc/distribution_spatial.h

+  double total_strength() { return this->point_idx_dist_.integral(); }
+
+private:
+  gsl::span<const double> x_, y_, z_;


To me it makes sense to use vector<Position> to reduce multiple indexing operations that may be prone to error if changes are made down the line.

pshriwise · 2024-10-09T02:04:17Z

openmc/stats/multivariate.py

+    psoitions: numpy.ndarray (3xN)
+        The points in space to be sampled


Suggested change

psoitions: numpy.ndarray (3xN)

The points in space to be sampled

positions: numpy.ndarray

The points in space to be sampled with shape (N, 3)

pshriwise · 2024-10-09T02:07:10Z

openmc/stats/multivariate.py

+        return cls(positions, strengths)
+
+
+


Suggested change

pshriwise · 2024-10-09T02:09:16Z

openmc/stats/multivariate.py

+        """
+        coord = {}
+
+        for axis in ('x','y','z'):


I'm thinking the XML format of the data as a flat list of points (X1, Y1, Z1, X2, Y2, Z2, ...) would make the import export code simpler, and it isn't more or less readable in an XML format (for my brain anyway).

pshriwise · 2024-10-09T02:11:58Z

include/openmc/distribution_spatial.h

+  double total_strength() { return this->point_idx_dist_.integral(); }
+
+private:
+  gsl::span<const double> x_, y_, z_;


One more thought, we should make sure to use a container here so the object owns the memory of these attributes -- gsl::span is just a view into memory space and doesn't "own" these values.

pshriwise · 2024-10-09T02:18:16Z

openmc/stats/multivariate.py

+
+        for idx, axis in enumerate(('x','y','z')):
+            subelement = ET.SubElement(element, axis)
+            subelement.text = ' '.join(str(e) for e in self.positions[idx,:])


Suggested change

subelement.text = ' '.join(str(e) for e in self.positions[idx,:])

subelement.text = ' '.join(str(e) for e in self.positions[..., idx])

I think?

paulromano

@gonuke Thanks for this PR! Out of curiosity, what is the intended application of this feature?

gonuke · 2024-10-10T17:52:12Z

We have a user who has an existing approximation of a volumetric source by a high density list of isotropic point sources. They want to use that source for comparison to other simulations.

gonuke · 2024-10-12T17:40:38Z

While it may have been elegant to add an istream operator for Position the version I added did not round-trip with the existing ostream operator and it is more convenient in XML to not use the verbose ostream format.

Thus, I've relied on just reading a list of doubles and packing them into Position in the reader.

… XML node

pshriwise

Some small line suggestions here @gonuke. I went ahead and added test that samples the external source distribution on the C++ side and factored out a function in the XML interface for parsing a set of Positions from a flat array of values on a node. Hope that's okay!

Thanks again for this addition.

docs/source/usersguide/settings.rst

openmc/stats/multivariate.py

pshriwise · 2024-10-15T18:57:01Z

openmc/stats/multivariate.py

+        if given_positions is None:
+            raise ValueError('No positions were provided')


I think I'd opt to let the check_iterable_type throw an error here instead.

pshriwise · 2024-10-15T19:33:04Z

openmc/stats/multivariate.py

+
+        element.set('type', 'cloud')
+
+        subelement = ET.SubElement(element, 'coords')


I believe the XML IO format spec will need to be updated to include this option.

src/distribution_spatial.cpp

openmc/stats/multivariate.py

…rance and number of particles.

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

gonuke added 8 commits October 5, 2024 10:51

Add PointCloud spatial distribution

c427b88

clang-format

93afabf

PointCloud should be handled like a standard IndependentSource in set…

954191d

…tings

clean up reference to mesh info in PointCloud

2077a35

missing commas

74532a7

add missing function declaration

0c78f39

dumb typo

a50d7af

remove stale enumeration

733337d

gonuke force-pushed the add_point_cloud branch from 1d86130 to 733337d Compare October 5, 2024 16:11

gonuke added 2 commits October 5, 2024 11:59

another dumb typo

3138b92

testing object equivalence is invalid

27ce9fc

gonuke marked this pull request as ready for review October 5, 2024 20:08

gonuke commented Oct 5, 2024

View reviewed changes

include/openmc/distribution_spatial.h Show resolved Hide resolved

gonuke commented Oct 5, 2024

View reviewed changes

pshriwise requested changes Oct 9, 2024

View reviewed changes

paulromano reviewed Oct 10, 2024

View reviewed changes

gonuke added 5 commits October 11, 2024 17:16

add input stream operator for Position

bbb2d4b

update XML of list of positions to be flattened list of coords

3f6e5e3

fix constructor arguments

3a5a9e6

linting

c016666

back away from istream operator for Position

f8933d7

gonuke and others added 4 commits October 12, 2024 12:42

I forgot to store the position data

85663d3

Create a new XML function for reading a flat set of Positions from an…

d8a0fd7

… XML node

Adding check for source strengths based using sampled source

2c7eb6c

C++ formatting

7f2b39a

PEP8

3641f99

pshriwise requested changes Oct 15, 2024

View reviewed changes

pshriwise and others added 6 commits October 16, 2024 10:10

Use a simpler model to cut down on initialization time. Increase tole…

5fafc7d

…rance and number of particles.

grammar fix

ed3b1d3

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

improve comment grammar

f4b5901

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

capitalization standard

3dd55a7

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

remove extra blank line

d9ca528

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

improve error message

ea462a9

Co-authored-by: Patrick Shriwise <pshriwise@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PointCloud spatial distribution #3161

Add PointCloud spatial distribution #3161

gonuke commented Oct 5, 2024 •

edited

Loading

gonuke Oct 5, 2024

pshriwise Oct 8, 2024

gonuke Oct 11, 2024

gonuke Oct 5, 2024

pshriwise Oct 17, 2024

gonuke Oct 5, 2024

gonuke Oct 5, 2024

gonuke Oct 5, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

pshriwise left a comment

pshriwise Oct 8, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

pshriwise Oct 9, 2024

paulromano left a comment

gonuke commented Oct 10, 2024

gonuke commented Oct 12, 2024

pshriwise left a comment

pshriwise Oct 15, 2024

pshriwise Oct 15, 2024

		psoitions: numpy.ndarray (3xN)
		The points in space to be sampled

	subelement.text = ' '.join(str(e) for e in self.positions[idx,:])
	subelement.text = ' '.join(str(e) for e in self.positions[..., idx])

		if given_positions is None:
		raise ValueError('No positions were provided')


		element.set('type', 'cloud')

		subelement = ET.SubElement(element, 'coords')

Add PointCloud spatial distribution #3161

Are you sure you want to change the base?

Add PointCloud spatial distribution #3161

Conversation

gonuke commented Oct 5, 2024 • edited Loading

Description

Checklist

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

pshriwise 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

paulromano left a comment

Choose a reason for hiding this comment

gonuke commented Oct 10, 2024

gonuke commented Oct 12, 2024

pshriwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke commented Oct 5, 2024 •

edited

Loading