Closed GaryLZW closed 10 months ago
Attention: 4 lines
in your changes are missing coverage. Please review.
Comparison is base (
2f656d7
) 86.27% compared to head (80b7e3e
) 86.49%.
Files | Patch % | Lines |
---|---|---|
carmm/analyse/counterpoise_onepot.py | 92.72% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi, I added an example to the regression test. I only managed to generate all the geometry.in files for now. I think this is now ready to commit.
Hi, I added an example to the regression test. I only managed to generate all the geometry.in files for now. I think this is now ready to commit.
It seems your test is failing though? We need to have a working QA test please! It can then be used as documentation for other people on how to use the code.
Hi, I changed my code yesterday trying to calculate the counterpoise correction directly. I am still testing it and I will add another comment once it is done.
Hi. This function is now able to calculate counterpoise correction directly. An example for CI-test is also updated. I didn't get it to work with socket though. I think this is now ready to merge.
Immediate comments:
More broadly, I’d prefer if this was not a single function to do everything, but a grouping of functions to achieve this activity. For example, a function that can return the appropriate subobjects for doing a BSSE, and then another function to do the calc. This makes things more flexible.
Hi. I rewrite the function into several smaller functions. The basis files I use now are empty ones but with the same file name as aims basis files. I also deleted the basis files in aims output files I am using. Is there any more copyright issue with using aims output files? Some comments are added to the example to make it more clear to new users.
Thanks. No issue with output files - they are not copyrighted.
Is there any more copyright issue with using aims output files?
I hope I can make it to do all the calculations and analyses too in future. Regression tests haven't been added yet.