Continuously Varying Material Tracking Method (CVMT) Implementation #1358

rockfool · 2019-09-20T17:47:49Z

This PR is to add CVMT method in the particle tracking. It allows the nuclides number densities in Material class be expressed by the functional expansion. However, the functional expansion tallied nuclides number densities is not determined. Any comment will be appreciated. I am also preparing the testing cases for this implementaion.

include/openmc/math_functions.h

gridley · 2019-09-25T00:21:55Z

Just wondering, how does this method generally perform compared to delta tracking?

rockfool · 2019-09-25T02:23:17Z

Just wondering, how does this method generally perform compared to delta tracking?
This method was come up as the alternate way of delta-tracking to sample the particle flight path in the medium with continously varying cross sections. However, delta-tracking might meet with low sampling efficiency issue and the track-length estimator issue. This is a direct sampling method, and has the most compatibility with ray-tracking.
[1]. Brown, Forrest B., and William R. Martin. "Direct sampling of Monte Carlo flight paths in media with continuously varying cross-sections." In Proc. ANS Mathematics & Computation Topical Meeting, vol. 2. 2003.

hanzhuoran · 2019-09-25T02:32:57Z

Just wondering, how does this method generally perform compared to delta tracking?
Delta-tracking could be faster than this direct sampling when XS doesn't vary too much in the domain. When both continuous number density and continuous XS sections are involved, it might be harder to evaluate maximum macroscopic XS.

gridley · 2019-09-25T02:37:37Z

Interesting, thanks for the replies guys. Has anyone tried making this hybrid with delta tracking, kind of like how Serpent does hybrid delta tracking and ray tracing?

rockfool · 2019-09-25T02:57:17Z

Interesting, thanks for the replies guys. Has anyone tried making this hybrid with delta tracking, kind of like how Serpent does hybrid delta tracking and ray tracing?

As I know, none of us is working on the delta-tracking or hybrid with delta-tracking currently.

paulromano

Thanks for the PR @rockfool. This branch looks like it needs considerable clean-up before it would be ready for review. Please have a look at our C++ style guide and try to conform to it. I also do not see any tests or documentation which will be needed as well. Once this has been cleaned up and tests/docs added, ping me for a review again. Thanks!

include/openmc/constants.h

include/openmc/material.h

include/openmc/particle.h

paulromano · 2019-09-30T19:43:31Z

src/particle.cpp

+      d_collision = -std::log(prn()) / macro_xs_.total; 
+
+      // cvmt: continuous varying materials tracking  
+      if(model::materials[material_]->continuous_num_density_) d_collision = sampling_cvmt(this, boundary.distance);


Be cognizant of line length, try to keep lines under 80 or 90 characters wide

paulromano · 2019-09-30T19:44:03Z

src/particle.cpp

+Particle::copy_data(LocalCoord &to, LocalCoord from){
+  to.r         = from.r        ;
+  to.u         = from.u        ;
+  to.cell      = from.cell     ;


Don't use extraneous whitespace between RHS and ;

paulromano · 2019-09-30T19:44:29Z

src/particle.cpp

+            this -> coord_[this -> n_coord_-1].u[1-1], 
+            this -> coord_[this -> n_coord_-1].u[2-1], 
+            this -> coord_[this -> n_coord_-1].u[3-1], 
+            this -> E_, ds);


All this debugging stuff should be removed

paulromano · 2019-09-30T19:44:46Z

src/particle.cpp

+              this -> coord_[this -> n_coord_-1].r[1-1],  
+              this -> coord_[this -> n_coord_-1].r[2-1], 
+              this -> coord_[this -> n_coord_-1].r[3-1], 
+              new_temp, xs_t[i-1]);


Debugging stuff should be removed

paulromano · 2019-09-30T19:45:08Z

src/particle.cpp

+        printf("delta_tau_hat = %15.7f", delta_tau_hat);
+        printf("index = %15d", index);
+        printf("ds*(index-1) = %15.7f", ds * (index-1));
+        printf("ds*(index) = %15.7f", ds * (index));


Remove all these printfs

rockfool · 2019-09-30T23:25:51Z

Thanks for the PR @rockfool. This branch looks like it needs considerable clean-up before it would be ready for review. Please have a look at our C++ style guide and try to conform to it. I also do not see any tests or documentation which will be needed as well. Once this has been cleaned up and tests/docs added, ping me for a review again. Thanks!

Thanks for your comments. I am going to remove those stuff that does not conform to C++ style, and try to prepare a testing case and the documentation in docs/neutron_physics.rst.

paulromano · 2020-01-27T13:31:40Z

Hi @rockfool. Just wanted to check in regarding this PR. If you'd like me to review this one again, please update it by merging the develop branch in order to resolve the existing merge conflicts.

rockfool · 2020-01-27T15:10:17Z

Hi @rockfool. Just wanted to check in regarding this PR. If you'd like me to review this one again, please update it by merging the develop branch in order to resolve the existing merge conflicts.

OK. I will try to reovle this conflict ASAP.

paulromano

@rockfool This pull request still needs a bit of work. A few general comments that apply throughout:

In many cases, you are declaring variables before you have a value to define them. This is a common source of uninitialized value bugs. Please move the variable declarations to the point of definition as well.
There are many places throughout your code that don't match our style guide. Rather than fixing one-by-one, what I'd suggest you do is to use clang-format along with the .clang-format file in the root directory of the repo to automatically apply formatting. That will save both of us the headache of addressing the minor style issues
It looks like the only polynomials that are supported are Zernike polynomials. I think to be considered a general capability, we would want to support the ability to set arbitrary polynomials in x, y, and z.
I don't see any support for this feature on the Python side; that will need to be added in order to be given full consideration for merging.

paulromano · 2020-02-06T20:30:54Z

include/openmc/material.h

+  double radius_;                          //! the radius of the expansion
+  std::vector<double> coeffs_;             //! coefficients for poly evaluation
+  std::vector<double> poly_norm_;          //! polynomial norm available to property for efficiency
+  public: 


NL.16: Put public members before private

paulromano · 2020-02-06T20:32:36Z

src/material.cpp

+  poly_norm_.resize(n_coeffs_);
+}
+
+PolyProperty::~PolyProperty()


This destructor is not needed -- when the object is destructed, the memory from the vectors will be removed

paulromano · 2020-02-06T20:37:09Z

src/material.cpp

+  return property;
+}
+
+PolyProperty::PolyProperty(int n_size)


I would have the constructor be:

PolyProperty::PolyProperty(const std::string& type, const std::vector<double>& coeffs)

and then have all the members (n_coeffs_, coeffs_, order_, type_) be set directly in the constructor rather than have that logic spill into the Material constructor.

paulromano · 2020-02-06T20:37:46Z

src/material.cpp

+{
+  int c_index;
+  double rho, poly_val, property;
+  rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];


Don't declare a variable until you have a suitable value for it:

Suggested change

rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];

double rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];

paulromano · 2020-02-06T20:40:52Z

src/material.cpp

+  int c_index;
+  double rho, poly_val, property;
+  rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];
+  c_index = 2;


Suggested change

c_index = 2;

int c_index = 2;

paulromano · 2020-02-06T20:46:38Z

src/particle.cpp

+  double ds, new_temp;
+  LocalCoord coord;
+
+  copy_data(coord, this->coord_[this->n_coord_ - 1]);


Pretty sure the following will work:

LocalCoord coord = this->coord_[this->n_coord_ - 1];

and then remove the copy_data method

paulromano · 2020-02-06T20:51:15Z

src/particle.cpp

+}
+
+void 
+Particle::estimate_flight_distance(std::vector<double> xs_t, double distance, double tau_hat, double& s)


This looks like it doesn't need to be a method of Particle and can be a plain function instead. Also, why not just return a double instead of having that last parameter?

paulromano · 2020-02-06T20:51:26Z

src/particle.cpp

+}
+
+void 
+Particle::get_quadratic_root(double a, double b, double c, double lower_b, double upper_b, double& root)


This looks like it doesn't need to be a method of Particle and can be a plain function instead

paulromano · 2020-02-06T20:51:32Z

src/particle.cpp

+}
+
+void 
+Particle::get_cubic_root(double a, double b, double c, double d, \


This looks like it doesn't need to be a method of Particle and can be a plain function instead

paulromano · 2020-02-06T20:53:36Z

include/openmc/particle.h

+  void simpsons_path_integration(double& optical_depth, double distance, std::vector<double> &xs_t, bool dbg_file, int it_num);
+  void estimate_flight_distance(std::vector<double> xs_t, double distance, double tau_hat, double& s);
+  void get_quadratic_root(double a, double b, double c, double lower_b, double upper_b, double& root);
+  void get_cubic_root(double a, double b, double c, double d, double lower_b, double upper_b, double& root);


For all these functions/methods, you need to add Doxygen comments above describing what the parameters are

rockfool added 3 commits September 10, 2019 12:30

add CVMT method in neutron transport

4cafc29

Merge remote-tracking branch 'upstream/develop' into cvmt

d056e52

fix a bug in cvmt sampling

de0ad46

hanzhuoran reviewed Sep 20, 2019

View reviewed changes

include/openmc/math_functions.h Outdated Show resolved Hide resolved

include/openmc/math_functions.h Outdated Show resolved Hide resolved

remove the duplicate zernike function

88cceb1

rockfool force-pushed the cvmt branch from 119c7bf to 88cceb1 Compare September 23, 2019 15:18

paulromano requested changes Sep 30, 2019

View reviewed changes

rockfool added 8 commits October 6, 2019 23:37

change fixed array to std::vector in material.h and cpp

592bcf0

remove debugging stuff

c8ce1e2

remove fet_order from settings.h

5920d73

add documentation for cvmt method

b3f4b83

change figures extension

3bd6bfb

add regression_test case for cvmt

227809f

modify function name in test.py

b26149f

change printf to warning function in error.h

dad6df9

rockfool added 4 commits January 27, 2020 11:32

resolve conflict in particle class

5f9c13f

bug fix in resolved confliction

63a10fa

remove useless function definition

cf34d86

fix the conflict on tally_scoring

c850e4a

paulromano requested changes Feb 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continuously Varying Material Tracking Method (CVMT) Implementation #1358

Continuously Varying Material Tracking Method (CVMT) Implementation #1358

rockfool commented Sep 20, 2019

gridley commented Sep 25, 2019

rockfool commented Sep 25, 2019

hanzhuoran commented Sep 25, 2019

gridley commented Sep 25, 2019

rockfool commented Sep 25, 2019

paulromano left a comment

paulromano Sep 30, 2019

paulromano Sep 30, 2019

paulromano Sep 30, 2019

paulromano Sep 30, 2019

paulromano Sep 30, 2019

rockfool commented Sep 30, 2019

paulromano commented Jan 27, 2020

rockfool commented Jan 27, 2020

paulromano left a comment

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

paulromano Feb 6, 2020

	rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];
	double rho = std::sqrt(r.x * r.x + r.y * r.y) / coeffs_[0];

Continuously Varying Material Tracking Method (CVMT) Implementation #1358

Are you sure you want to change the base?

Continuously Varying Material Tracking Method (CVMT) Implementation #1358

Conversation

rockfool commented Sep 20, 2019

gridley commented Sep 25, 2019

rockfool commented Sep 25, 2019

hanzhuoran commented Sep 25, 2019

gridley commented Sep 25, 2019

rockfool commented Sep 25, 2019

paulromano 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

rockfool commented Sep 30, 2019

paulromano commented Jan 27, 2020

rockfool commented Jan 27, 2020

paulromano 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