-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generalize summary generation #1054
Generalize summary generation #1054
Conversation
d176956
to
50e5fcd
Compare
50e5fcd
to
63b0428
Compare
@AndresOrtegaGuerrero this one's next. Let me fix the tests first. I'll mark as ready when done. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
+ Coverage 67.84% 67.87% +0.02%
==========================================
Files 112 112
Lines 6581 6593 +12
==========================================
+ Hits 4465 4475 +10
- Misses 2116 2118 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I would suggest to add an extra test, that it doesnt consider the default values, to check changes are being used. I would say that we should try to merge first #1055 first since that one is causing the app to not work with different periodicity systems. |
I suspect merging #1055 first will cause a massive conflict headache with this one. But I can adjust if you think it is absolutely required. |
We can then try this one first , and you add more test in #1055 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The test with DFT+U , spin and XYZ periodicity works well. However, we can continue testing in the other PR, focusing on the point-group, and add more tests in that PR to further generalize different options.
This PR aims to simplify workflow summary maintenance by generalizing the template and introducing a schema. Adding new fields reduces to adding the field in the report dictionary and schema.