openmopac / mopac

Molecular Orbital PACkage
http://openmopac.net
GNU Lesser General Public License v3.0
115 stars 31 forks source link

November updates from Jimmy & minor bug fix #136

Closed godotalgorithm closed 1 year ago

godotalgorithm commented 1 year ago

These are some updates from Jimmy Stewart, and a fix for some minor bugs reported by a MOE developer. The main change is that the keyword parser should no longer be confused when valid keywords are contained within string-valued arguments of other keywords. Unfortunately, the input parser in MOPAC is unnecessarily complicated, and these changes are not making it any simpler. The changes are non-trivial, and I don't have good testing coverage of them. While I'm accepting this PR when all of the existing tests are passed, I would not be surprised if this has silently broken something that will need to be fixed in the near future.

Status

godotalgorithm commented 1 year ago

The code-coverage CI run is triggering an extremely weird bug in a large fraction of the tests where DATA CURRENTLY READ IN ARE: is printed just before the status print-out at the end of a MOPAC output file. This print statement exists in one location of the code that can only trigger when the input file is being read, so there is no valid way for it to trigger right at the end of the program. This could be related to other recently reported erratic problems in the last few print statements of the output file, but I'm not clear how I can effectively debug this remotely. I might ignore this until I can reproduce it in a local environment, perhaps by mimicking various details of this GHA environment.

codecov-commenter commented 1 year ago

Codecov Report

Merging #136 (8a33fc3) into main (7e4b992) will increase coverage by 0.00%. The diff coverage is 59.61%.

@@           Coverage Diff           @@
##             main     #136   +/-   ##
=======================================
  Coverage   66.67%   66.68%           
=======================================
  Files         331      331           
  Lines       73757    73725   -32     
=======================================
- Hits        49180    49162   -18     
+ Misses      24577    24563   -14     
Impacted Files Coverage Δ
src/PARAM/parkey.F90 0.00% <0.00%> (ø)
src/input/getdat.F90 42.91% <0.00%> (-1.67%) :arrow_down:
src/interface/mopac_gui.F90 25.00% <0.00%> (ø)
src/output/to_screen.F90 48.90% <0.00%> (-0.19%) :arrow_down:
src/run_param.F90 0.00% <0.00%> (ø)
src/models/switch.F90 60.48% <5.00%> (ø)
src/models/refer.F90 58.53% <20.00%> (-0.48%) :arrow_down:
src/input/readmo.F90 55.26% <33.33%> (+0.25%) :arrow_up:
src/input/getgeo.F90 60.04% <50.00%> (+2.31%) :arrow_up:
src/output/pdbout.F90 88.46% <50.00%> (ø)
... and 38 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

godotalgorithm commented 1 year ago

The weird bug turned out to be varying behavior between compilers when read reaches the end of a file. Most compilers seem to treat multiple read commands at the end of a file as an end-of-file event. However, gfortran now treats any read commands after the first end-of-file event as a file-read error with status 5001. Ideally, MOPAC would avoid further file reads upon reaching the end of a file. However, that would require a rather careful examination to fix thoroughly, and I'm instead redirecting the error back to the end-of-file logic branch when the appropriate error status is detected.