Repository: alchemistry/alchemlyb (language: python)

Issue:
Behaviour of amber parser on file with duplicated time
In many occasions, one would need to run simulation in chunks. For 10 ns simulation, one run the first ns, the seconds ns and so on. For Gromacs, this is easy and the output would be a continuous file. However, given that AMBER doesn't have a checkpoint continue mechanism. A convention to do continuation would be that one generate a number of amber output files and concatenate the text files together. 
This would result in

```
 NSTEP =      200   TIME(PS) =      40.400  TEMP(K) =   300.75  PRESS =     0.0
 Etot   =    -17617.2822  EKtot   =      3869.1100  EPtot      =    -21486.3921
 BOND   =         7.5660  ANGLE   =       100.4182  DIHED      =         6.3933
 1-4 NB =         6.9516  1-4 EEL =       -34.1688  VDWAALS    =      3077.9556
 EELEC  =    -24653.8963  EHBOND  =         0.0000  RESTRAINT  =         2.3882
 EAMBER (non-restraint)  =    -21488.7803
 EKCMT  =         0.0000  VIRIAL  =         0.0000  VOLUME     =     65829.0102
                                                    Density    =         0.9878
 TEMP0  =       298.0000  REPNUM  =              1  EXCHANGE#  =              1
 ------------------------------------------------------------------------------

...

MBAR Energy analysis:
Energy at 0.0000 =    -21510.840230
Energy at 0.0667 =    -21507.963581
Energy at 0.1333 =    -21490.171333
Energy at 0.2000 =    -21448.494884
Energy at 0.2667 =    -21379.504007
Energy at 0.3333 =    -21284.274183
Energy at 0.4000 =    -21167.309140
Energy at 0.4667 =    -21035.636518
Energy at 0.5333 =    -20897.816290
Energy at 0.6000 =    -20762.961241
Energy at 0.6667 =    -20640.032587
Energy at 0.7333 =    -20537.016108
Energy at 0.8000 =    -20460.060524
Energy at 0.8667 =    -20412.262820
Energy at 0.9333 =    -20391.474269
Energy at 1.0000 =    -20388.088606
 ------------------------------------------------------------------------------
...
NSTEP =      200   TIME(PS) =      40.400  TEMP(K) =   300.75  PRESS =     0.0
 Etot   =    -17617.2822  EKtot   =      3869.1100  EPtot      =    -21486.3921
 BOND   =         7.5660  ANGLE   =       100.4182  DIHED      =         6.3933
 1-4 NB =         6.9516  1-4 EEL =       -34.1688  VDWAALS    =      3077.9556
 EELEC  =    -24653.8963  EHBOND  =         0.0000  RESTRAINT  =         2.3882
 EAMBER (non-restraint)  =    -21488.7803
 EKCMT  =         0.0000  VIRIAL  =         0.0000  VOLUME     =     65829.0102
                                                    Density    =         0.9878
 TEMP0  =       298.0000  REPNUM  =              1  EXCHANGE#  =              1
 ------------------------------------------------------------------------------

...

MBAR Energy analysis:
Energy at 0.0000 =    -21510.840230
Energy at 0.0667 =    -21507.963581
Energy at 0.1333 =    -21490.171333
Energy at 0.2000 =    -21448.494884
Energy at 0.2667 =    -21379.504007
Energy at 0.3333 =    -21284.274183
Energy at 0.4000 =    -21167.309140
Energy at 0.4667 =    -21035.636518
Energy at 0.5333 =    -20897.816290
Energy at 0.6000 =    -20762.961241
Energy at 0.6667 =    -20640.032587
Energy at 0.7333 =    -20537.016108
Energy at 0.8000 =    -20460.060524
Energy at 0.8667 =    -20412.262820
Energy at 0.9333 =    -20391.474269
Energy at 1.0000 =    -20388.088606
 ------------------------------------------------------------------------------
```

So you have duplicated time points in the same input file. 
The current behaviour is that when we reach the end of the first file `5. TIMINGS`, we close the file parser, so the rest of the information is gone, which is not ideal.

There are several ways of solving this problem.
The easiest way would be to just to remove this break https://github.com/alchemistry/alchemlyb/blob/master/src/alchemlyb/parsing/amber.py#L361
This would allow the parser to continue. On the other hand, instead of logging the data for `0~1 ns, 0~1 ns, 0~1 ns`. This would log 0`~1 ns, 1~2 ns, 2~3 ns`. In some sense this would mean that we are not reading the data as it is. On the other hand, this would probably be the expected behaviour in this case so this might be fine.

The more involved way of solving this problem might be to log the data as it is, For example, `0~1 ns, 0~1 ns, 0~1 ns`. Then the user might need to change the time point by themselves.

I'm in favour of the easy solution but is not sure of this would create some additional problem @DrDomenicoMarson 
Here is an example input file , note that only 50 data points are loaded.
[amber.out.zip](https://github.com/alchemistry/alchemlyb/files/11531471/amber.out.zip)




Discussion / hints:
Hi @xiki-tempula ,
In dealing with the exact situation you are describing  I usually just read each output file from the same simulation and concatenate them via `concat` from `alchemlyb`, without any issue!

The `time` for each file is not directly read from the output file, but is computed from the starting time, the `dt`, and the `steps between output`. 

The only problem may arise if one tries to concatenate files from different simulations, but this is the case also for GROMACS I think!

@DrDomenicoMarson Thanks. 
Do you think it would be fine if I just remove https://github.com/alchemistry/alchemlyb/blob/master/src/alchemlyb/parsing/amber.py#L361
Then issue a warning if there are more data after the finish?
I don't have the time to test this right now, but I think it shouldn't be an issue!

I think there should be an Error and not a Warning, if data is found after the finish because I don't know if the logic that is used now would deal with a single file that is a concatenation of several output AMBER files though.  
Close this via the concatenation of the pandas dataframe #316
An error should be thrown if the input is a concatenated amber output file

Fix patch:
diff --git a/CHANGES b/CHANGES
index c3ed0b3e..37d88468 100644
--- a/CHANGES
+++ b/CHANGES
@@ -18,6 +18,8 @@ The rules for this file:
   * 2.1.0
 
 Changes
+  - ValueError raised if concatenated amber output file is passed to amber
+    parser (issue #315, PR #326).
   - Change the % based string formatting to {} based string formatting (issue #323, PR #324).
   - Use loguru instead of logging for log (issue #301, PR #303).
 
diff --git a/src/alchemlyb/parsing/amber.py b/src/alchemlyb/parsing/amber.py
index 543d4d54..6bb5441f 100644
--- a/src/alchemlyb/parsing/amber.py
+++ b/src/alchemlyb/parsing/amber.py
@@ -332,10 +332,18 @@ def extract(outfile, T):
                     "^ NSTEP", "^ ---", ["NSTEP", "DV/DL"], extra=line
                 )
                 if nstep != old_nstep and dvdl is not None and nstep is not None:
+                    if finished:
+                        raise ValueError(
+                            "TI Energy detected after the TIMINGS section. Did you concatenate the output file?"
+                        )
                     file_datum.gradients.append(dvdl)
                     nensec += 1
                     old_nstep = nstep
             elif line.startswith("MBAR Energy analysis") and file_datum.have_mbar:
+                if finished:
+                    raise ValueError(
+                        "MBAR Energy detected after the TIMINGS section. Did you concatenate the output file?"
+                    )
                 mbar = secp.extract_section(
                     "^MBAR", "^ ---", file_datum.mbar_lambdas, extra=line
                 )
@@ -356,7 +364,6 @@ def extract(outfile, T):
                     )
             elif line == "   5.  TIMINGS\n":
                 finished = True
-                break
 
         if high_E_cnt:
             logger.warning(


Test patch:
diff --git a/src/alchemlyb/tests/parsing/test_amber.py b/src/alchemlyb/tests/parsing/test_amber.py
index 0d186cc7..21b6064e 100644
--- a/src/alchemlyb/tests/parsing/test_amber.py
+++ b/src/alchemlyb/tests/parsing/test_amber.py
@@ -1,6 +1,7 @@
 """Amber parser tests.
 
 """
+import bz2
 import logging
 
 import pandas as pd
@@ -250,3 +251,35 @@ def test_u_nk_improper(improper_filename, names=("time", "lambdas")):
         assert u_nk.index.names == names
     except Exception:
         assert "0.5626" in improper_filename
+
+
+def test_concatenated_amber_u_nk(tmp_path):
+    with bz2.open(load_bace_example()["data"]["complex"]["decharge"][0], "rt") as file:
+        content = file.read()
+
+    with open(tmp_path / "amber.out", "w") as f:
+        f.write(content)
+        f.write("\n")
+        f.write(content)
+
+    with pytest.raises(
+        ValueError,
+        match="MBAR Energy detected after the TIMINGS section.",
+    ):
+        extract(tmp_path / "amber.out", 298)
+
+
+def test_concatenated_amber_dhdl(tmp_path):
+    with bz2.open(load_bace_example()["data"]["complex"]["decharge"][0], "rt") as file:
+        content = file.read().replace("MBAR Energy analysis", "")
+
+    with open(tmp_path / "amber.out", "w") as f:
+        f.write(content)
+        f.write("\n")
+        f.write(content)
+
+    with pytest.raises(
+        ValueError,
+        match="TI Energy detected after the TIMINGS section.",
+    ):
+        extract(tmp_path / "amber.out", 298)