feat: add sevennet support to mlp recipes #2553
Conversation
|
Can one of the admins verify this patch? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2553 +/- ##
=======================================
Coverage 97.32% 97.32%
=======================================
Files 85 85
Lines 3550 3554 +4
=======================================
+ Hits 3455 3459 +4
Misses 95 95 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@mamunm: Thank you very much for your PR! I have just two questions before I can merge this.
src/quacc/recipes/mlp/_base.py
Outdated
| if "model" not in kwargs: | ||
| kwargs["model"] = "7net-0" |
There was a problem hiding this comment.
7net-0 is the default model, so it seems like we should probably not specify this.
There was a problem hiding this comment.
I added both the default settings so the user knows what model and devices are being used if they want to dig deeper. But it's redundant, and I'll remove them in the next push.
There was a problem hiding this comment.
The default setting is "auto" for device I believe, but in any case let's remove them so we are always consistent with the underlying calculator. If there are changes to the model, it should (ideally) break the tests here.
src/quacc/recipes/mlp/_base.py
Outdated
| if "device" not in kwargs: | ||
| kwargs["device"] = "cpu" |
There was a problem hiding this comment.
The default device is "auto". Is there a reason to override this default by forcing CPU?
|
Thanks, merging! |
Thanks for the review! |
Summary of Changes
Added sevennet to the MLP recipes based on issue #2429.
Key changes include:
Requirements
-[X] My PR is focused on a single feature addition [#2429 ].
-[X] My PR has relevant, comprehensive unit tests.
-[X] My PR is on a custom branch (feature/sevennetMLP).