PSDP Algorithms #198
PSDP Algorithms #198
Conversation
Codecov Report
@@ 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
|
|
We can keep it in |
procrustes/psdp.py
Outdated
| @@ -34,10 +34,90 @@ | |||
| __all__ = [ | |||
| "psdp_woodgate", | |||
| "psdp_peng", | |||
| "psdp_opt" | |||
| "psdp_opt", | |||
| "psdp_projgrad" | |||
There was a problem hiding this comment.
"psdp_projgrad",It's a good practice to have a comma to make the Git history clean.
There was a problem hiding this comment.
In the end we may wish to split off some helper functions. The key criterion is that helper functions that are generically useful (not tired to pdsp) can be split off, provided we can make them "generic" enough to be useful outside the pdsp context. Otherwise better to keep encapsulated with the rest of the pdsp code.
It's really a matter of style. In the bad old days of programming, people deal with very long files. Nowadays people prefer shorter files, but if the number of files gets too large, it can be difficult to find what you are looking for in the code. Ideal, I think, is to hopefully make it so that when you need to find something, you can guess which file it will be in with two-three guesses, which suggests relatively few files, but not too few.
There was a problem hiding this comment.
Thanks for the suggestions. It makes a lot of sense to me.
|
Once you fix the testing errors and you think it's ready for code review, please tag me. Thanks. @abhishekmittal15 |
|
Hi @FanwangM , sure will do that. |
Improved readability of psdp_projgrad and added explicit stop conditions for the algorithm
abhishekmittal15
force-pushed
the
psdp-algorithms
branch
from
d40741b
to
288e678
Compare
|
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 |
|
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. |
|
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 |
|
That's strange. Can you check if you are using the same version of linters as that is in GitHub Actions? |
|
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 |
This is because 1. actions/setup-python#544 2. Python 3.6 has came to end of its life and not longer supported
procrustes/psdp.py
Outdated
| if e1 < e2 or choice == 1: | ||
| return S1 | ||
| elif e2 < e1 or choice == 2: | ||
| return S2 |
There was a problem hiding this comment.
The return should be a ProcrustesResult object. Also, we need an empty line at the end of the file as a convention.
procrustes/psdp.py
Outdated
| @@ -34,10 +34,90 @@ | |||
| __all__ = [ | |||
| "psdp_woodgate", | |||
| "psdp_peng", | |||
| "psdp_opt" | |||
| "psdp_opt", | |||
| "psdp_projgrad" | |||
There was a problem hiding this comment.
Thanks for the suggestions. It makes a lot of sense to me.
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?