-
Notifications
You must be signed in to change notification settings - Fork 8
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
Count FLOPs and HitRate inside functor and output them to a log file #850
base: master
Are you sure you want to change the base?
Conversation
…eterised and tests globals
# Conflicts: # src/autopas/LogicHandler.h
applicationLibrary/molecularDynamics/molecularDynamicsLibrary/LJFunctor.h
Outdated
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/molecularDynamicsLibrary/LJFunctor.h
Outdated
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/molecularDynamicsLibrary/LJFunctor.h
Outdated
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/molecularDynamicsLibrary/LJFunctor.h
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/tests/singleSiteTests/ForceCalculationTest.cpp
Outdated
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/tests/singleSiteTests/LJFunctorFlopCounterTest.cpp
Outdated
Show resolved
Hide resolved
applicationLibrary/molecularDynamics/tests/templateInstatiations/AutoPasInstantiations.cpp
Outdated
Show resolved
Hide resolved
AutoPasLog(WARN, "LJFunctorAVX::getHitRate called but is not implemented and will return 0."); | ||
return 0; | ||
} | ||
|
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.
Sorry to be so annoying about the warnings.
But my default setup when I'm not interested in flops would use the LJ-AVX functor with AUTOPAS_LOG_FLOPS=OFF
and log-level=warn.
This leads to a completely full terminal output :(
I'm also not so sure how to handle this if you really want a runtime warning within the functor...
Maybe the return value could be std::numeric_limits<double>::quiet_NaN()
, to make it very clear in the output that these are no correct values?
3864582
to
7fa8112
Compare
…LOPs or getHitRate. FLOPLogger makes such cases clearer by outputting blank spaces. md-felxible does not handle FLOP logging problems itself
To summarise a recent change to this branch:
|
* leaves a blank space in the log. | ||
* @return number of FLOPs | ||
*/ | ||
[[nodiscard]] virtual int getNumFLOPs() const { return -1; } |
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 should return a 64-bit value like long
, otherwise, I fear we risk overflows here. Also, why sacrifice one full bit for the error state? Why not simply use 0
or std::numeric_limits<size_t>::max()
(=2^64 - 1)?
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.
Could use std::numeric_limits<size_t>::max()
. I think 0
is too potentially problematic.
* | ||
* @return (number of kernel calls) / (number of distance calculations) | ||
*/ | ||
[[nodiscard]] virtual double getHitRate() const { return -1; } |
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.
Here I'd go for a special double
value like std::numeric_limits<T>::quiet_NaN
(not sure if there is something more fitting).
const auto numFLOPsStr = numFLOPs >= 0 ? std::to_string(numFLOPs) : ""; | ||
const auto hitRateStr = hitRate >= 0 ? std::to_string(hitRate) : ""; | ||
spdlog::get(_loggerName)->info("{},{},{}", iteration, numFLOPsStr, hitRateStr); |
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.
What's the reason you are logging anything at all in that case and not just skip the call to the logger?
Something like:
const auto numFLOPsStr = numFLOPs >= 0 ? std::to_string(numFLOPs) : ""; | |
const auto hitRateStr = hitRate >= 0 ? std::to_string(hitRate) : ""; | |
spdlog::get(_loggerName)->info("{},{},{}", iteration, numFLOPsStr, hitRateStr); | |
const auto numFLOPsStr = numFLOPs >= 0 ? std::to_string(numFLOPs) : ""; | |
const auto hitRateStr = hitRate >= 0 ? std::to_string(hitRate) : ""; | |
// Only write if at least one value is not empty | |
if (not numFLOPsStr.empty() or not hitRateStr.empty())) { | |
spdlog::get(_loggerName)->info("{},{},{}", iteration, numFLOPsStr, hitRateStr); | |
} |
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 don't want to skip the logger because it makes it clear that the functor was still called but that no metrics were provided, as opposed to that it wasn't called. Furthermore, consider the scenario that two functors are used, one of which has one function implemented only and the other has none (niche scenario, but illustrates wider point). One functor results in a log with one empty field, implying that empty fields are outputted, and the other doesn't log anything.
Ultimately, I don't think the scenario where the user has AUTOPAS_LOG_FLOPS=ON
but is using a functor which has no implementation is that important - I just want something which works and is not going to leave the user wondering why the FLOP log makes no sense,
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.
consider the scenario that two functors are used,
For that case you should probably also write the functor name in every line or create one log file per functor.
if (_loggedEmptyFields) { | ||
AutoPasLog(INFO, |
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 this should be a warning. It's not spammy and if the user is annoyed by it they should disable the feature they did not implement because that is improper utilization.
- Maybe also write this message the first time this happens otherwise a user will only read this at the end of their simulation.
- I'm unsure if I would like this message only at the start, only the end, or both. Pro/Cons:
- Start: Can be seen quickly after starting the simulation leading to better reaction times but easy to miss.
- End: Harder to miss but very late.
- Both: Best of both worlds but duplicated warning.
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.
You're a bit too premature with this comment haha - it wasn't ready for review and this doesn't really apply anymore.
FYI, this didn't actually work, I think, because the AutoPas logger gets deregistered before this object is destructed. So I opted with a much more heavy-handed approach of just logging "Not Implemented".
The Start doesn't work because we would need to force the user simulator to tell the logger which functors it will use (unless I am missing something). This could be problematic.
Option 2 could work, but might be problematic if this is in the depths of the log file and not that noticeable. As a WARN
this might not be too bad.
I will post a comment summarising this further change.
DocTagCheckerUnchanged DocumentationThe following doc files are unchanged, but some related sources were changed. Make sure the documentation is up to date!
|
Just to summarise the even more recent change: FLOPLogger now logs "Not Implemented" in the CSV field if a functor does not implement
|
Description
The FLOP counter functor was removed as it was too rigid. See #841 for more details on this "rigidity". The proposed alternative for counting FLOPs was for it to be handled internally by a functor. The advantages are outlined in #841. This has been implemented.
At the same time, md-flexible's FLOP outputting mechanic was misleading. See #707 for more details. A solution to this was sketched as an idea in #695.
This pull request
LJFunctor
. Implementing the FLOP counting mechanism inside the other vectorisations of this functor is not worthwhile, as their future is questionable in the face of potential vectorised wrapper variants, as discussed in Intrinsics Wrappers #833. TheMultisiteLJFunctor
will not get an implementation in this PR, as the implementation of the functor is suboptimal and will be replaced in Multi Site LJ AVX512 Functor #810.Related Pull Requests
Resolved Issues
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
LJFunctor
's FLOP counter.