check leftover FIXMEs in r2018
A number of FIXME entries were left behind (late merge, quite heavy last-minute testing/fixing), some related to PME GPU, others not. We should check and eliminate these asap in the master branch unless they hide bugs.
A similar check could be done for leftover TODOs.
#1 Updated by Szilárd Páll 6 months ago
Some of these are clear candidates to look at:
$ git diff origin/release-2016 | grep "FIXME" +# Re-enable Phi build (FIXME: do we still care?) + * FIXME: It would be better to refactor so that + std::shared_ptr<PmeGpuSpecific> archSpecific; /* FIXME: make it an unique_ptr */ + // FIXME verify that the box is scaled correctly on GPU codepath + wallcycle_sub_start_nocount(wcycle, ewcsLAUNCH_GPU_PME); //FIXME nocount + /* FIXME: + //FIXME - this is box[XX][XX] * box[YY][YY] * box[ZZ][ZZ], should be stored in the PME structure - /* FIXME: The TNG atoms should contain mass and atomB info (for free + * FIXME: Atom B state data should also be written to TNG (v 2.0?) */ + /* FIXME after 5.0: Currently only standard block types are read */ - /* FIXME after 5.0: Currently only standard block types are read */ /* FIXME: Undefined which plugin is chosen if more than one plugin /* Calculate self corrections to the GB energies - currently only A state used! (FIXME) */ +// FIXME: remove the "__" prefix in front of the group def when we move the - /* FIXME: It would be better not to have the string here hardcoded. */ + // FIXME: MD and EM separately set up the local state - this should happen in the same function, + // FIXME: this is only here to manually unpin mdAtoms->chargeA_ and state->x, + # PME tests; FIXME: move this back into mdrun_test_objlib above and figure out the MPI race issue + // FIXME: without this barrier, one of the mdruns was somehow having a non-PME inputrec (!)
#2 Updated by Aleksei Iupinov 6 months ago
There is nothing critical in the PME ones, just some ugliness and leftovers. I treated "FIXME" as a "TODO with slightly higher priority" :-)
The last 2 lines are removed in the chain that re-enables separate PME rank tests, up from https://gerrit.gromacs.org/#/c/7469/.
BTW, grepping release-2016 diff for FIXME gives 19 results, while grepping for TODO - 382.
#7 Updated by Mark Abraham 6 months ago
Aleksei Iupinov wrote:
That was about HostAllocationPolicy residing in gpu_utils instead of utility.
Actually, shouldn't gpu_utils be a part of utility?
Not even sure if "util*" is a fitting description for such basic building blocks.
There's some need to reconsider what is in utility. (There was some discussion about its interaction with the SIMD module for a SIMD-suitable array view.)There's cases for separating
- a module of low-level things used in many places (memory, error handling, logging, strings, assertions, exceptions, mutex, variant, regex; timing we already separated)
- a module of miscellaneous things used rarely (pretty printing, version reporting)
- a module for stream or I/O or serialization support
- a module for key-value tree support
- a module for GPU operation support (transfers, launches)
Some things in gpu_utils might make sense in the "low level things," some not.