theochem / procrustes

Python library for finding the optimal transformation(s) that makes two matrices as close as possible to each other.
https://procrustes.qcdevs.org/
GNU General Public License v3.0
109 stars 20 forks source link

PSDP Algorithms #198

Closed abhishekmittal15 closed 1 year ago

abhishekmittal15 commented 1 year ago

Hi @FanwangM , I have implemented the 1st algorithm mentioned in #194 , the Projected Gradient method and written the tests for it by taking the input matrices from the woodgate algorithm tests and the error percentages are better in certain cases. I will fix the docstrings soon and start implementing the Fast Gradient method. Looking forward to your feedback on the current code. Also I had a question regarding the file psdp.py, is it advisable to add all the algorithms to the same file, as the file is already getting really large, maybe we can shift some helper functions somewhere else and probably create a folder for PSDP and put each algorithm into its own file? What do you suggest we do?

codecov[bot] commented 1 year ago

Codecov Report

Merging #198 (ea5941b) into master (025426f) will increase coverage by 0.08%. The diff coverage is 96.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   94.58%   94.67%   +0.08%     
==========================================
  Files          11       11              
  Lines         757      807      +50     
==========================================
+ Hits          716      764      +48     
- Misses         41       43       +2     
Impacted Files Coverage Δ
procrustes/psdp.py 95.54% <96.00%> (+0.14%) :arrow_up:
FanwangM commented 1 year ago

We can keep it in psdp.py for now. Please tag me once you think it's ready for review. I just put two rough comments. Thank you for contributing! @abhishekmittal15

FanwangM commented 1 year ago

Once you fix the testing errors and you think it's ready for code review, please tag me. Thanks. @abhishekmittal15

abhishekmittal15 commented 1 year ago

Hi @FanwangM , sure will do that.

abhishekmittal15 commented 1 year ago

Hi @FanwangM , I think you can now review my projection gradient implementation. There are some linting tests that fail and I am not sure exactly why its failing. The force push above was because I had squashed some commits into one, rewriting the previous pushed commit, hope thats not an issue

FanwangM commented 1 year ago

That's not an issue at all. But you can fix the linter errors by checking out the detailed log at https://github.com/theochem/procrustes/actions/runs/3496401782/jobs/5854272424 by googling the error numbers, for example "procrustes/psdp.py:197:10: N806 variable 'F' in function should be lowercase". For something you are unsure of, you can google it and find people has some posts already, especially in StackOverflow. @abhishekmittal15

Thanks.

abhishekmittal15 commented 1 year ago

Hi, I am not sure if thats the reason the linting is failing. Because when I run linter locally, then it shows lint errors (most common error is "variable name doesnt conform to snake case styling") even in the psdp_opt, psdp_peng etc, but these implementations passed the checks.

FanwangM commented 1 year ago

That's strange. Can you check if you are using the same version of linters as that is in GitHub Actions?

abhishekmittal15 commented 1 year ago

Sorry for the delay in getting back to you, I had my end semester exams and projects. I have fixed all of the linting errors, but one remains and that is the C0302: Too many lines in module (1035/1000) (too-many-lines). What are your thoughts on how to proceed ahead with this error