Feature/dlw components 3 #49
Conversation
|
|
||
| "LP", | ||
| &Block::LP, | ||
| "The primary flag value (0, 1 or 2)" |
There was a problem hiding this comment.
[design question, future]
Should these return enums or similar instead?
There was a problem hiding this comment.
Yes, we probably should do that. I'll make that into an issue.
| ) | ||
| .def_property_readonly( | ||
|
|
||
| "number_bins", |
There was a problem hiding this comment.
[aside]
I almost got caught out by number_bins not being NET and was going to comment their documentation is different.
| class Test_ACEtk_GeneralEvaporationSpectrum( unittest.TestCase ) : | ||
| """Unit test for the GeneralEvaporationSpectrum class.""" | ||
|
|
||
| chunk = [ 0, |
There was a problem hiding this comment.
[clarification]
I'm a bit confused on the value of NR. In the ACE format document, it indicates that if NR = 0, then you use linear-linear interpolation.
Is this related to the multi-law stuff and thus I won't see any tests with a non-zero value until then? It is tricky to figure out what this does from the format manual.
There was a problem hiding this comment.
It seems to be an option in the ACE format but I've never seen anything that uses NR != 0. We can talk about this in more detail when we go over these things after my updates following the review.
| { return value.incidentEnergy(); }; | ||
|
|
||
| std::sort( distributions.begin(), distributions.end(), | ||
| [=] ( const auto& left, const auto& right ) |
There was a problem hiding this comment.
I don't think the [=] is doing anything as all the variables I see used are passed as arguments, but if it were, would it be better to [&]?
There was a problem hiding this comment.
The incidentEnergy lambda should get captured here.
| xss[index] = std::visit( [] ( const auto& value ) | ||
| { return value.incidentEnergy(); }, | ||
| distribution ); | ||
| xss[index] = std::visit( incidentEnergy, distribution ); | ||
|
|
||
| // set the locator |
There was a problem hiding this comment.
[non-PR aside]
Should this comment note that it is LOCC? I was baffled by it setting a negative value until I tracked that down.
There was a problem hiding this comment.
I updated the comment to be more clear on what is happening
| /** | ||
| * @brief Return the primary flag value (0, 1 or 2) | ||
| */ | ||
| unsigned int primaryPhotonFlag() const { return this->LP(); } |
There was a problem hiding this comment.
[question]
LP=2 has different kinematics than LP=0 or 1 (and takes the excess kinetic energy from an incident particle, making it "primary"). What distinguishes LP=0 from LP=1?
| // reserve size in the vector | ||
| std::size_t nr = boundaries.size(); | ||
| std::size_t ne = distributions.size(); | ||
| std::vector< double > xss( 2 + ne ); // NE, list of energies, NET |
There was a problem hiding this comment.
[comment]
This way of constructing XSS appears to be 100% correct. However, I had to spend more time than I'd like proving it to myself due to the nonlinearity going on (allocate, prepend, set values, append).
| int numberBins() const { return this->length() - 1; } | ||
|
|
||
| /** | ||
| * @brief Return the cosine values |
There was a problem hiding this comment.
I corrected this. I also checked the python side where it seems to be correct.
| GeneralEvaporationSpectrum& operator=( const GeneralEvaporationSpectrum& base ) { | ||
|
|
||
| new (this) GeneralEvaporationSpectrum( base ); | ||
| return *this; |
There was a problem hiding this comment.
[question]
How do you prevent memory leaks caused by repeatedly doing:
GeneralEvaporationSpectrum& x = y;
and letting x go out of scope - say, in a loop?
[Edit] did some research, did not know about placement-new. In discussions, people note that since new is not exception-safe, this can create malformed objects on exception. Also, it may not be well-defined if you do x = x. Which is odd but possible.
There was a problem hiding this comment.
I probably should look into this some more.
Three more energy distribution options: