lanl / hippynn

python library for atomistic machine learning
https://lanl.github.io/hippynn/
Other
59 stars 22 forks source link

Generalize ASE DB import. #33

Closed mgt16-LANL closed 11 months ago

mgt16-LANL commented 1 year ago

Hi Nick,

I tried to update the ASE DB parser to handle the "allow_unfound" keyword. I'll update with a generalized extended-xyz parser as well before this should be merged.

I picked up developing this PR since discussions with people at IPAM revealed more generalized input database structures aligned with ASE standards would likely be generally useful.

-Michael

mgt16-LANL commented 1 year ago

Just added the XYZ parsers for ASE objects. Let me know if you want me to send an example set for each to show the functionality.

lubbersnick commented 1 year ago

This is cool! I notice a lot of shared code between the two classes. Would it be possible to either make a superclass for the shared code that these two types have, or make one class a subclass of the other? As a small but similar note, I think these things fit together well enough to live in the same file.

Also, as things are now, ase is not a required dependency. With this change, the regular numpy and dictionary database classes would not be usable without ASE installed. I think we can find a simple way to make this feature easy to use if you have ASE installed, but not get in the way if ASE isn't present.

Lastly, a bit of examples/documentation would be great. Definitely a note in the changelog.

mgt16-LANL commented 1 year ago

Would it make sense to you, structurally, to move these under the hippynn/interfaces/ase_interface folder? This might keep the ASE code segmented into one place, but might be harder to locate for someone looking to import ase databases - we could add a comment in the main database import that there are ASE database importers over in this folder. I'm happy make the other changes - making these into a single class should be relatively simple - I could hard code detection of database type based on extension? LMK what you think!

mgt16-LANL commented 1 year ago

The other way I've seen handling optional dependencies is to try/except import of the package and adjust the database definitions based on the presence of the package - but I often find this pretty inelegant: https://stackoverflow.com/questions/563022/whats-python-good-practice-for-importing-and-offering-optional-features

lubbersnick commented 1 year ago

Would it make sense to you, structurally, to move these under the hippynn/interfaces/ase_interface folder?

Yes, that would make sense to me.

The other way I've seen handling optional dependencies is to try/except import of the package and adjust the database definitions based on the presence of the package

These two strategies can be combined; if the root database setup finds ase, it can get those objects and add them to the database all? Or maybe that's overcomplicated. We already do have some code that uses try+import in the custom kernels package, it seems to work well enough. I think the key here not to change definitions based on the availability, rather just do more imports from subpackages when those things are available.

mgt16-LANL commented 1 year ago

Hey! I updated the example to be the one you pointed me to - I think it is pretty seamless plugin - I had to hard-code the energy_per_atom as a name, though. I haven't waiting for the model to finish training - but after 55 epochs this was the table produced:

`Epoch 55: Learning rate: 0.001 Training time: 12.0 s
Validating... train valid

EpA-RMSE : 3.6176 3.7732 EpA-MAE : 3.1217 3.1888 EpA-RSQ : 0.94312 0.93441 ForceRMSE: 108.63 131.9 ForceMAE : 82.3 * 83.744 ForceRsq : 0.97891 0.96903 T-Hier : 0.00064398 0.00065772 L2Reg : 60.443 60.443 Loss-Err : 0.25832 0.28526 Loss-Reg : 0.00070442 0.00071817 Loss : 0.25903 0.28598 Best Loss-Err so far: 0.25786 Epochs since last best: 1 Current max epochs: 104 Total epoch time: 14.22 s`

mgt16-LANL commented 1 year ago

Let me know if the documentation/example/changelog should be cleaner - I tried to add it where it made sense to me and match your style - but I'm not sure if it was entirely successful.

mgt16-LANL commented 1 year ago

I guess if it's useful I ran the original script https://github.com/lanl/benchmarks/blob/main/kokkos_lammps_hippynn/train_model.py for comparison and found nearly identical behavior in training:

`Epoch 53: Learning rate: 0.001 Training time: 48.95 s
Validating... train valid

EpA-RMSE : 3.2861 3.465 EpA-MAE : 2.81 2.8493 EpA-RSQ : 0.94487 0.94469 ForceRMSE: 131.76 120.8 ForceMAE : 86.53 * 87.369 ForceRsq : 0.96885 0.97402 T-Hier : 0.00065143 0.00065415 L2Reg : 54.939 54.939 Loss-Err : 0.27925 0.27132 Loss-Reg : 0.00070637 0.00070909 Loss : 0.27996 0.27202 Best Loss-Err so far: 0.25489 Epochs since last best: 1 Current max epochs: 102 Total epoch time: 53.24 s`