Project

General

Profile

Task #2379

check leftover FIXMEs in r2018

Added by Szilárd Páll 9 months ago. Updated 8 months ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Difficulty:
uncategorized
Close

Description

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.


Related issues

Related to GROMACS - Bug #2385: PME mixed mode produces incorrect results with box scalingClosed

History

#1 Updated by Szilárd Páll 9 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 9 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.

#3 Updated by Aleksei Iupinov 9 months ago

  • Related to Bug #2385: PME mixed mode produces incorrect results with box scaling added

#4 Updated by Aleksei Iupinov 9 months ago

Oh well, there was a missing box scaling call for mixed PME :-)

#5 Updated by Mark Abraham 9 months ago

There was also at least one cycle suppression introduced because we didn't find a good way to resolve some dependency between mdtypes and gpu_utils module. Think it was 3755f7d2ce2f0b10896733e5fc613d8ad0bab4fe

#6 Updated by Aleksei Iupinov 9 months ago

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.

#7 Updated by Mark Abraham 9 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.

Also available in: Atom PDF