Closed ntBre closed 1 year ago
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.
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.
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.
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.
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.
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.
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.
Merging #125 (d278fc6) into main (6034fc3) will increase coverage by
0.20%
. The diff coverage is54.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%) |
:arrow_up: |
src/input/wrtkey.F90 | 64.14% <100.00%> (+0.02%) |
:arrow_up: |
src/run_mopac.F90 | 77.74% <100.00%> (ø) |
|
src/SCF/iter.F90 | 89.03% <0.00%> (+0.16%) |
:arrow_up: |
src/input/readmo.F90 | 55.01% <0.00%> (+0.44%) |
:arrow_up: |
src/mopend.F90 | 78.12% <0.00%> (+1.56%) |
:arrow_up: |
src/input/getgeo.F90 | 57.72% <0.00%> (+1.62%) |
:arrow_up: |
src/output/to_screen.F90 | 49.09% <0.00%> (+12.32%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
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.
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.
This PR introduces the
INTERNAL
keyword, which is similar to theEXTERNAL
keyword, except it looks for a secondINTERNAL
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 newINTERNAL
keyword with the same parameters.Status
The main issue I see is that it would be nice to factor out the commonality between
datin
anddatex
(that name might need to be changed too), but for now I just copy-pasteddatin
and made the changes I needed.