Feature/mf33 - Githubissues

nathangibson14 commented 3 years ago

Built off current develop, should be fun when we get all the 20s into develop, too. A few clashes expected 😢

Complete implementation of MF33 in both C++ and Python.

whaeck commented 3 years ago

Alea iacta est ...

nathangibson14 commented 3 years ago

I just renamed Subsection to ReactionBlock, NIType to ExplicitCovariance, and NCType to DerivedCovariance. Any complaints with those?

-- Nathan A. Gibson Staff Scientist, XCP-5 Nuclear Data Team Los Alamos National Laboratory (505) 665-2338

From: Wim Haeck @.> Reply-To: njoy/ENDFtk @.> Date: Monday, July 26, 2021 at 5:12 PM To: njoy/ENDFtk @.> Cc: "Gibson, Nathan Andrew" @.>, Author @.***> Subject: [EXTERNAL] Re: [njoy/ENDFtk] Feature/mf33 (#141)

@whaeck requested changes on this pull request.

Nicely done. Some of the interface function names are debatable (looking at NC and NI) - as we've discussed previously - but we can change these at a later stage.

Basically, the remarks are for consistency in documentation and testing:

add to_string tests on the python side
use keyword arguments on the python side
documentation on the C++ side

In python/src/file/33.python.cpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r675845575:

+

+// declarations - sections

+void wrapSection_33( python::module&, python::module& );

+

+void wrapFile_33( python::module& module, python::module& viewmodule ) {

+

// type aliases
using Section = njoy::ENDFtk::section::Type< 33 >;
using File = njoy::ENDFtk::file::Type< 33 >;
using SectionRange = BidirectionalAnyView< Section >;
// create the submodule
python::module submodule = module.def_submodule(
"MF33",
"MF33 - covariances of neutron cross sections"

You should drop the neutron since these could also be for incident charged particles - I wager.

In python/test/Test_ENDFtk_MF33_CovariancePairs.pyhttps://github.com/njoy/ENDFtk/pull/141#discussion_r675850152:

self.assertEqual( 4, chunk.number_values )

self.assertEqual( 2, chunk.NP )
self.assertEqual( 2, chunk.number_pairs )
values
self.assertAlmostEqual( 1., chunk.Ek[0] )
self.assertAlmostEqual( 2., chunk.Fk[0] )
self.assertAlmostEqual( 3., chunk.Ek[1] )
self.assertAlmostEqual( 4., chunk.Fk[1] )
self.assertAlmostEqual( 1., chunk.first_array_energies[0] )
self.assertAlmostEqual( 2., chunk.first_array_fvalues[0] )
self.assertAlmostEqual( 3., chunk.first_array_energies[1] )
self.assertAlmostEqual( 4., chunk.first_array_fvalues[1] )
self.assertEqual( 2, chunk.NC )

to_string should go here

In src/ENDFtk/section/33/Subsection/src/readNI.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676062690:

Log::info( "MT value: {}", MT );

Log::info( "Line number: {}", lineNumber - 1 );
Log::info( "LB value: {}", list.L2() );

Some nitpicking: output the LB value after the list of legal LB values. The MT line is not required.

In src/ENDFtk/section/33/Subsection/src/readNC.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676062735:

Log::info( "MT value: {}", MT );

Log::info( "Line number: {}", lineNumber - 1 );
Log::info( "LTY value: {}", cont.L2() );

Some nitpicking: output the LTY value after the list of legal LTY values. The MT line is not required.

In src/ENDFtk/section/33/Subsection/src/ctor.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676062967:

+Subsection( Iterator& begin,

const Iterator& end,
long& lineNumber,
int MAT, int MF, int MT )

+try :

Subsection(
ControlRecord( begin, end, lineNumber, MAT, MF, MT ),
begin, end, lineNumber, MAT, MF, MT ) {

nitpicking: alignment of code

In src/ENDFtk/section/33/Subsection/src/ctor.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676063216:

Log::info( "Trouble while reading section {} of File 33 of Material {}",

MT, MAT );

This is a Subsection, so the message is inappropriate (and it would be added twice once the try ... catch in the actual MF33 section is encountered). A better message would be:

Log::info( "Encountered error while constructing a covariance subsection" );

In src/ENDFtk/section/33/Subsection/src/ctor.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676063255:

NI Only.

*
*/

Argument documentation is missing.

In src/ENDFtk/section/33/Subsection/src/ctor.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676063348:

NC Only.

*
*/

Argument documentation is missing

In src/ENDFtk/section/33/Subsection/src/NC.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676063471:

@@ -0,0 +1,21 @@

+long NC() const {

nitpicking: documentation is missing

In src/ENDFtk/section/33/Subsection.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676063943:

double xmf1_;

double xlfs1_;

You should make these integers, or even unsigned integers. It's not because the ENDF format says that it is stored as a double that you need to interpret it as a double. Conversion to double is done when writing out the file.

In src/ENDFtk/section/33/Subsection.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676064119:

double XMF1() const { return this->xmf1_; }

+

/**
- @brief Return the file number for the second cross section
*/
double secondFileNumber() const { return this->XMF1(); }
/**
- @brief Return the final excited state for the second cross section
*/
double XLFS1() const { return this->xlfs1_; }
/**
- @brief Return the final excited state for the second cross section
*/
double secondFinalExcitedState() const { return this->XLFS1(); }
/**
- @brief Return the material number for the second cross section
*/
double MAT1() const { return this->mat1_; }
/**
- @brief Return the material number for the second cross section
*/
double secondMaterialNumber() const { return this->MAT1(); }
/**
- @brief Return the section number for the second cross section
*/
double MT1() const { return this->mt1_; }
/**
- @brief Return the section number for the second cross section
*/
double secondSectionNumber() const { return this->MT1(); }

These return types should be integers. You're actually converting the mat1 and mt1 to doubles from integers. I assume this is a copy/paste error.

In src/ENDFtk/section/33/Subsection.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676064339:

@brief Return the section number for the second cross section

*/
double secondSectionNumber() const { return this->MT1(); }
/**
- @brief Return the number of NC-type subsubsections
*/
int NK() const { return this->nc_.size(); }
/**
- @brief Return the number of NC-type subsubsections
*/
int numberNCType() const { return this->NK(); }
/**
- @brief Return the number of NC-type subsubsections

This is for NI, not NC

In src/ENDFtk/section/33/Subsection.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676064362:

+

/**
- @brief Return the number of NI-type subsubsections
*/
int numberNIType() const { return this->NI(); }
/**
- @brief Return the NC-type subsubsections
*/
auto componentsNC() const {
return ranges::cpp20::views::all( this->nc_ );
}
/**
- @brief Return the NC-type subsubsections

this is for NI, not NC

In src/ENDFtk/section/33/DerivedRedundant/src/NC.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676064808:

@@ -0,0 +1,4 @@

+/**

- @brief Return the number of lines in this MF15 component

this is an MF33 component

In src/ENDFtk/section/33.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676064919:

+#include "range/v3/range/conversion.hpp"

+#include "range/v3/view/all.hpp"

+#include "range/v3/view/concat.hpp"

+#include "range/v3/view/drop_exactly.hpp"

+#include "range/v3/view/take_exactly.hpp"

+#include "range/v3/view/stride.hpp"

+#include "ENDFtk/ControlRecord.hpp"

+#include "ENDFtk/ListRecord.hpp"

+#include "ENDFtk/readSequence.hpp"

+#include "ENDFtk/section.hpp"

+

+namespace njoy {

+namespace ENDFtk {

+namespace section{

+

+namespace hana = boost::hana;

Where are you using hana?

In python/src/section/33/CovariancePairs.python.cpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676992832:

+

python::init< int,
std::vector< double >&&, std::vector< double >&& >(),
python::arg( "lb" ),
python::arg( "energies" ), python::arg( "fvalues" ),
"Initialise the component\n\n"
"Arguments:\n"
" lb covariance procedure\n"
" energies energies\n"
" fvalues F-values\n"
)
.def_property_readonly(
"LT",
&Component::LT,
"the number of pairs in the second array"

Now I'm really nitpicking: you should use a capital T since it's like that everywhere else in ENDFtk

In python/test/Test_ENDFtk_MF33_CovariancePairs.pyhttps://github.com/njoy/ENDFtk/pull/141#discussion_r676995047:

self.assertEqual( 2, chunk.number_pairs )

+

values
self.assertAlmostEqual( 1., chunk.Ek[0] )
self.assertAlmostEqual( 2., chunk.Fk[0] )
self.assertAlmostEqual( 3., chunk.Ek[1] )
self.assertAlmostEqual( 4., chunk.Fk[1] )
self.assertAlmostEqual( 1., chunk.first_array_energies[0] )
self.assertAlmostEqual( 2., chunk.first_array_fvalues[0] )
self.assertAlmostEqual( 3., chunk.first_array_energies[1] )
self.assertAlmostEqual( 4., chunk.first_array_fvalues[1] )
self.assertEqual( 2, chunk.NC )
the data is given explicitly
chunk = CovariancePairs(1, [1, 3], [2, 4])

For these tests I would use the keywords to make sure they are correctly set in the bindings.

In python/test/Test_ENDFtk_MF33_Section.pyhttps://github.com/njoy/ENDFtk/pull/141#discussion_r676995795:

the data is retrieved from a tree element and parsed

tape = Tape.from_string( self.valid_TPID + self.chunk +
self.valid_SEND + self.valid_FEND +
self.valid_MEND + self.valid_TEND )
chunk = tape.material( 9437 ).file( 33 ).section( 2 ).parse()
verify_chunk( self, chunk )

You're not verifying limp when parsing from a tape?

In src/ENDFtk/section/33/CovariancePairs.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676996610:

long numberValues() const { return this->NT(); }

+

/**
- @brief Return the total number of pairs
*/
long NP() const { return this->NT()/2; }
/**
- @brief Return the total number of pairs
*/
long numberPairs() const { return this->NP(); }
/**
- @brief Return the energy values from the first array
*/
auto Ek() const {

ENDF speak should be all caps, even if the ENDF manual uses something differently. Just for consistency.

In src/ENDFtk/section/33/CovariancePairs/test/CovariancePairs.test.cpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676998418:

+// other includes

+

+// convenience typedefs

+using namespace njoy::ENDFtk;

+using CovariancePairs = section::Type< 33 >::CovariancePairs;

+

+std::string chunkLB1();

+std::string invalidLB();

+std::string inconsistentNT();

+void verifyChunkLB1( const CovariancePairs& );

+void verifyChunkLB3( const CovariancePairs& );

+

+SCENARIO( "CovariancePairs" ) {

+

GIVEN( "valid data for a CovariancePairs" ) {

You should test LB=1 and LB=3 using separate GIVEN parts (see for instance ContinuumENergyAngle test)

In src/ENDFtk/section/33/DerivedRatioToStandard/src/NC.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676998583:

@@ -0,0 +1,4 @@

+/**

- @brief Return the number of lines in this MF15 component

This is MF33

In src/ENDFtk/section/33/DerivedRatioToStandard/src/ctor.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r676999660:

@@ -0,0 +1,80 @@

+//! @todo pybind11 variant needs default constructor workaround

+#ifdef PYBIND11

+/**

- @brief Default constructor - only enabled for pybind11
*/

+DerivedRatioToStandard() = default;

+#endif

+

+/**

- @brief Cascade for buffer iterator
*
- Needs to be public for reading from Subsection.

I'll have to look more closely at this.

In src/ENDFtk/section/33/Subsection/src/readNC.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r677001946:

@@ -0,0 +1,48 @@

+template< typename Iterator >

+static std::vector< NCType >

+readNC( Iterator& begin,

const Iterator& end,
long& lineNumber,
int MAT, int MF, int MT,
int numberNC ) {
std::vector< NCType > result;
for (int i=0; i<numberNC; ++i) {
ControlRecord cont( begin, end, lineNumber, MAT, MF, MT );

The only thing you need off here is the L2 value right?

Use that as an argument to the read constructor instead of moving an essentially useless CONT record into the components.

In src/ENDFtk/section/33/Subsection/src/readNI.hpphttps://github.com/njoy/ENDFtk/pull/141#discussion_r677002569:

@@ -0,0 +1,53 @@

+template< typename Iterator >

+static std::vector< NIType >

+readNI( Iterator& begin,

const Iterator& end,
long& lineNumber,
int MAT, int MF, int MT,
int numberNI ) {
std::vector< NIType > result;
for (int i=0; i<numberNI; ++i) {
ListRecord list( begin, end, lineNumber, MAT, MF, MT );

Here you're warranted to move the ListRecord into the components, because they are the components. With the CONT record in the NC subsections, it's not the same.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/njoy/ENDFtk/pull/141#pullrequestreview-714100779, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAK6KQ7NC3L763IB3XRW4ALTZXTXLANCNFSM5A4UGJRA.

nathangibson14 commented 3 years ago

This should be ready to go. I address at least most of the comments, and then found a few of my own issues (e.g., missing a constructor for CovariancePairs on the Python side. Looks like the CI passes again.

njoy / ENDFtk

Feature/mf33 #141

values

values

the data is given explicitly

the data is retrieved from a tree element and parsed