Closed justinGilmer closed 3 years ago
Merging #435 (a283ab6) into master (3b91082) will decrease coverage by
0.01%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #435 +/- ##
==========================================
- Coverage 73.59% 73.58% -0.02%
==========================================
Files 17 17
Lines 1803 1802 -1
==========================================
- Hits 1327 1326 -1
Misses 476 476
Looks like this reproduces the issue (test_load_metadata_from_internal_name
, the one using Forcefield(name="")
is failing).
I took Alex's suggested patch (https://github.com/mosdef-hub/foyer/issues/434#issuecomment-870754409) and made a couple fixes so that tests would pass. I haven't thought much about unintended side effects yet.
Here is the commit; feel free to move it around, copy, etc. : https://github.com/mosdef-hub/foyer/commit/22855d422f5770f596356e3cb0d48b49a899f3b9
Yeah we essentially had the same design process and handling of the strings. Once this passes and we have the approvals, we can merge. Thanks @mattwthompson and @ahy3nz for both of your contributions!
Does it matter that you're using preprocessed_files
and I'm using all_files_to_load
? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?
(Sorry for the misclick 😬)
Does it matter that you're using
preprocessed_files
and I'm usingall_files_to_load
? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?
That was my thought, we already strip out/handle things that might not behave the way we expect once we have pre-processed.
/azp run
Does it matter that you're using preprocessed_files and I'm using all_files_to_load? Both seem to pass tests ... it's probably better to use the pre-processed ones in principle?
That was my thought, we already strip out/handle things that might not behave the way we expect once we have pre-processed.
While I agree the pre-processed XML files are safer to try reading, we might have to consider this block of code that is being deleted in this PR. Originally, IIRC @justinGilmer ran into some problems with /tmp
blowing up when running a lot of atomtyping and creating a lot of preprocessed XML files in /tmp
, so we needed to add this code chunk to remove old XML files. If this chunk of code is removed, this problem may come up again.
https://github.com/mosdef-hub/foyer/commit/748bdd8646342e7a253ee3d3c9944eaad6b83cc9
try:
super(Forcefield, self).__init__(*preprocessed_files)
finally:
for ff_file_name in preprocessed_files:
os.remove(ff_file_name)
I did add a small section at the end of this init method to iterate through the open files and os.remove them. It runs into the problem where I cant really put a try catch finally around the entire part of that code to always ensure that the files are deleted, but it should handle most cases.
My mistake! Good catch
PR Summary:
Previously, the tests in test_forcefield.py were assumed to test that forcefield metadata like
version
,name
, etc. were properly loaded into theForcefield
object.This is only partially true, as there are cases where the files either go out of scope before that information is gathered, etc. This PR includes additional tests to ensure that this information is not being lost prematurely. Big thanks to: @mattwthompson, @ahy3nz for discovering this issue, creating MWE's for some of these cases, and taking my investigation further and most likely pinpointing the issue.
PR Checklist
Refer to issues/PRs: #433 #434
Note: This does not have @ahy3nz 's fix incorporated yet, just the test cases that fail and currently work.