Read parameters from the input file

[ntBre]

This PR introduces the INTERNAL keyword, which is similar to the EXTERNAL keyword, except it looks for a second INTERNAL in the input file and reads the parameters from there instead of an external file.

I also added a test for the EXTERNAL keyword itself and for the new INTERNAL keyword with the same parameters.

Status

• [ x] Ready for merge

The main issue I see is that it would be nice to factor out the commonality between datin and datex (that name might need to be changed too), but for now I just copy-pasted datin and made the changes I needed.

[godotalgorithm]

I recall our discussions about this feature from the MQM conference this summer. While I welcome this expanded functionality, I'm not sure that it makes sense to attach it to a new keyword. An alternative approach would be to have the EXTERNAL keyword act like this INTERNAL keyword when used without a filename. If this is acceptable to you, then I can try to merge your datex.F90 back into datin.F90 and integrate the filename-less behavior. I'd like feedback from you before I attempt to do this, in case I am missing something and there is further reason to have a distinct INTERNAL keyword.

[ntBre]

That totally makes sense. I was having trouble getting started and only made progress after starting with a new keyword, but I agree that it's not necessary in hindsight. I can also try to work on it if you want, but I would definitely welcome you merging them back together as well.

[godotalgorithm]

Since this is your contribution, it would probably be best if you handle the merging of INTERNAL and EXTERNAL, but I can certainly assist as necessary.

Also, this new feature will add yet another possible data block after the geometry information in the input file. There are several other keywords that already do this (SYMMETRY for example), and I don't think the input parser is capable of reading multiple data blocks simultaneously when more than one of these keywords is active. Rather than fix the parsing (which would be a complicated mess), I can put together a list of keywords that read data blocks from the input file and create a barrier in wrtkey.F90 that prevents multiple data blocks from appearing in the input file. I can handle this part, but it's something that I would like to be a part of this PR - I'd like to avoid changes that make the input parser more fragile.

[ntBre]

Sounds good, I will take a shot at that today.

I did wonder about adding another block at the end of the file. Originally I even tried including all of the parameters as the "filename" following EXTERNAL=, but then they can't include newlines and I couldn't figure out how to read them from there anyway. I'm not sure how SYMMETRY reads its input, but I was hoping wrapping the parameters between INTERNAL and END (or something similar) would help with any conflicts. That barrier seems like a good idea to prevent potential conflicts, regardless. I'm definitely happy for you to handle that part, but I could potentially take a look too if you pointed me in the right direction. Whatever is easier for you!

On the other hand, I could try adding test cases combining these keywords if you wanted, depending on how many there are. From the documentation, I think SYMMETRY in particular might be okay, but I don't know how many other keywords could also conflict. I think I saw in some of the tests that multiple geometries can also be specified, which I didn't know about, so I hope this change works with that too.

[ntBre]

On a related note, is there a way to run a subset of the tests? Some of the tests take a bit longer than I would like on my laptop, and I haven't figured out yet how to run only some of them. As you probably saw from my issue yesterday, I'm not familiar with cmake, although it looks very cool so far.

[godotalgorithm]

Data blocks from older features in MOPAC do not have delimiters, which is what makes the parsing complicated. I'm trying to preserve backwards compatibility as much as possible, so I'd strongly prefer not to change the behavior of old keywords. However, in adding a new data block to the input file, there is nothing stopping you from adding a delimiter, which would certainly be for the better. Since you are merging this with the EXTERNAL keyword, the delimiters BEGIN EXTERNAL and END EXTERNAL would make sense.

CTest does have various filters on tests, so you can certainly use it to run only a subset of tests. The one wrinkle is that the number labeling each test isn't considered to be meaningful, so the tests are only filterable by their names. ctest --help is helpful for this, and either test -R <regex> or test -L <regex> should allow you to only run the tests that you are actively developing.

[ntBre]

Thanks for the ctest help!

I just added a commit merging INTERNAL and EXTERNAL and updated the tests. Should I change the name of the INTERNAL test too?

I'm also a bit stuck on how to fix the weird character in the INTERNAL.out test output. I tried checking that line at line 1576 of wrtkey.F90 was not equal to "" or " " and also tried checking its len_trim, but the len_trim comes out to 1, which I still couldn't check since it would conflict with an actual single-character filename. I also think this poses potential issues if the EXTERNAL= isn't the last keyword. When I moved it into the middle of the keyword line, the output reports the filename as the following keyword, but the results still appear to be correct. So it might just be a printing issue like the other case, but it probably still needs to be fixed.

[codecov-commenter]

Codecov Report

Merging #125 (d278fc6) into main (6034fc3) will increase coverage by 0.20%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   66.46%   66.67%   +0.20%     
==========================================
  Files         331      331              
  Lines       73723    73736      +13     
==========================================
+ Hits        49001    49162     +161     
+ Misses      24722    24574     -148     

Impacted Files	Coverage Δ
src/PARAM/parkey.F90	0.00% <0.00%> (ø)
src/run_param.F90	0.00% <0.00%> (ø)
src/input/datin.F90	72.28% <56.41%> (+2.63%)	⬆️
src/input/wrtkey.F90	64.14% <100.00%> (+0.02%)	⬆️
src/run_mopac.F90	77.74% <100.00%> (ø)
src/SCF/iter.F90	89.03% <0.00%> (+0.16%)	⬆️
src/input/readmo.F90	55.01% <0.00%> (+0.44%)	⬆️
src/mopend.F90	78.12% <0.00%> (+1.56%)	⬆️
src/input/getgeo.F90	57.72% <0.00%> (+1.62%)	⬆️
src/output/to_screen.F90	49.09% <0.00%> (+12.32%)	⬆️

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

[godotalgorithm]

I needed to make some changes, but everything looks reasonable now. The datin changes are a little ugly (i.e. use of goto), but input parsing in MOPAC is already an ugly mess, so this is consistent with the status quo.

[ntBre]

Thanks for cleaning it up, and for your help! Sorry about the gotos, I had never seen cycle or exit in the old code I've worked on, but they could probably be replaced with that if you prefer.

[godotalgorithm]

No problem, and thanks for your patience in my delays in getting to this PR review. I'm fine with things like goto in MOPAC as they are stylistically consistent with how most of the code is written. What I don't tolerant is unnecessary code duplication (hence the request to merge INTERNAL and EXTERNAL keywords) and archaic Fortran like the arithmetic if and computed goto syntax, which I have fully removed from MOPAC.