search

shirtsgroup / InterMol

Conversion tool for molecular simulations
MIT License
187 stars 54 forks source link

Only raise error for missing LAMMPS when called #324

Closed ctk3b closed 7 years ago

ctk3b commented 7 years ago

Also remove a few unused variables and fix stress test runs on Travis

Resolves #317

mrshirts commented 7 years ago

Are the CI tests failures expected at this point?

ctk3b commented 7 years ago

They are not and look like legitimate errors. I'll look into this tonight.

To recap, currently, the CI runs all of the conversions in test_all.py but without energy comparisons. Those should not be raising any errors even if they may produce output with non-matching energy.

mrshirts commented 7 years ago

Reasonable level of checking to do for now. I'm going to try to check in some energy checking things tonight. I think I've debugged most of the issues with Xubo Lin's files.

mrshirts commented 7 years ago

OK, submitted a PR that should resolve some of the issues (perhaps not all yet).

ctk3b commented 7 years ago

Did you push this to your fork?

mrshirts commented 7 years ago

Should be submitted as a PR (https://github.com/ctk3b/InterMol/pull/1) to your energies branch.

ctk3b commented 7 years ago

Ah got it. Apparently I don't get notifications for that...

ctk3b commented 7 years ago

There were a few issues with the stress tests. I've fixed them locally - we'll see what Travis thinks.

@mrshirts could you double check me on this desmond fix? https://github.com/shirtsgroup/InterMol/pull/324/commits/6b6cf17f873926f50f68fd161a69d03c463ad862 The error that it was generating was 

                if 'HOH' in type:
>                   dHH = float(split[6])
E                   ValueError: could not convert string to float: 'HOH'
ctk3b commented 7 years ago

The desmond fix I implemented appears to be the issue raised here https://github.com/shirtsgroup/InterMol/issues/256

ctk3b commented 7 years ago

Ok CI seems to be fixed.

@mrshirts if you're ok with the Desmond fix mentioned above then I think this can be merged and we can get #308 merged as well

mrshirts commented 7 years ago

Sounds good. Can you think of any potential complications with "Install openmm instead of simtk.units"?

ctk3b commented 7 years ago

Since we only use simtk.units, it should not be a problem. I think a good way to minimize external dependencies would be to just switch over to using the units package in parmed since we already require that as a dependency.