-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/initial v2 #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at this a lot more next week, but here are some scattered comments for now.
|
||
if ( this->metadata_.numberGroups().has_value() ) { | ||
|
||
readRecord( this->structure_, iter, end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've changed my mind. Instead of structure or group structure, energyBounds is probably the most accurate name for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made changes to these, check the latest version to see if these changes are OK for you. Basically, every reocrd like this will have a values() that returns the data while the table has a corresponding primaryGroupStructure() or similar function. As a result, the interface will look like:
table.primaryGroupStructure().values()
table.outgoingGroupStructure( 0 ).values()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outgoingGroupStructure only applies for secondary particles? And is ( 0 ) index 0, or saying particle type 0 (gamma)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Particle id, so 0 for gamma and 1001 for protons. I distinguish primary and outgoing structures. The primary structure is for the primary particle, aka the projectile and I assume the structure for the outgoing projectile type is the same as the primary structure (hence the scattering matrix being square while the production ones aren;t necessarily square)
*/ | ||
auto values() const { | ||
|
||
return this->values_ | njoy::tools::std20::views::all; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unfamiliar with "|" in C++. What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a C++20 addition. While we use C++17, this was added in the C++20 extension from njoy::tools (it replaces range-v3). The | operator (or pipe operator) is shorthand for:
return njoy::tools::std20::views::all_view( this->values_ );
While in this case it offers little advantage, there are cases where it adds readability:
auto f = [] ( auto&& value ) { /* do something */ };
return this->values_ | njoy::tools::std20::views::drop( 5 )
| njoy::tools::std20::views::transform( f );
which means: "drop the first 5 elements from this->values_ and then apply the transformation f to it". This is equivalent to:
auto f = [] ( auto&& value ) { /* do something */ };
return njoy::tools::std20::views::transform_view( njoy::tools::std20::views::drop_view( this->values_, 5 ), f );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My new concern is that some machines won't have C++20 things. Maybe they do, it's 2024. Do you know if Rocinante can handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is indeed a c++20 extension, it was implemented in the tools library using c++17 standard programming. The interface is in line with the c++20 specs. I have tested this even on intel-classic compilers and it works. You should not need a C++20 compatible compiler, you need a c++17 compatible one. Once we can actually switch standards, we can do away with all this.
One of the things we have to do soon is extend the testing capabilities we have to test more OS types and compilers. I plan on talking to the MCNP team about that.
std::string chunkWithInsufficientNumberCrossSectionValues() { | ||
|
||
return "sig_reac\n" | ||
" 2 0\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some cross sections to one of these reactions? So it's like 2 groups for one, and 0 groups for the other.
auto full = this->data().size() / 5; | ||
auto partial = this->data().size() - full * 5; | ||
auto x = this->data().begin(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance that in the future we will want to write out more or fewer than 5 entries per line? If yes, it might be worth our while now to set some #cols constant value we can change in one place. Write an internal loop within while( full -- ) to loop over #cols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but magic numbers are to be avoided anyway so why not change it? Other than global consts I don't see an easy way to do this though.
The same would go for the output precision too.
std::ostringstream buffer; | ||
|
||
auto full = this->data().size() / 5; | ||
auto partial = this->data().size() - full * 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
purely a taste thing here, but to me
auto partial = this->data().size() % 5;
is clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
A preliminary prototype implementation of NDItk for reading, writing and modifying the multigroup neutron and photon NDI tables.
The capability is limited but it illustrates how most of the toolkit will work. We cover the basic cases that we should encounter while reading records:
The python bindings are currently missing, but I will add these shortly.