-
Notifications
You must be signed in to change notification settings - Fork 22
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
Core class cleanup #387
Core class cleanup #387
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.
Only comment relates to the units used for clock frequency. So far as I can tell, there isn't a good reason for using GHz in some places and Hz in others. In which case, it would seem better to stick to one unit throughout the code base. However, @FinnWilkinson @jj16791 @dANW34V3R @JosephMoore25, please let me know if I'm missing something
You'd be correct in the use cases primarily converting the input GHz value into Hz. A significant reason behind the input being in GHz is that that's the natural unit of measure for modern HPC CPU cores. It's very unnatural to think about the frequency in terms of Hz and we'd need to add comments and notes to ensure that the user diverts from the natural assumptions. We kept it as GHz and got the code to convert it to Hz to make it "easier"/more convenient for the user. |
Is there a reason for using Hz in the code at all though? We could store |
19a83c7
to
7962ced
Compare
f9bc92a
7962ced
to
f9bc92a
Compare
…, fix for issue 386.
eedc657
f9bc92a
to
eedc657
Compare
Cleans up the Core classes by moving common member variables into the base class and ensuring the code aligns with the rest of the project.
Closes #356
Fixes #386