Skip to content
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

Update #68

Merged
merged 3 commits into from
Mar 9, 2021
Merged

Update #68

merged 3 commits into from
Mar 9, 2021

Conversation

FarnazH
Copy link
Member

@FarnazH FarnazH commented Mar 8, 2021

  1. The symmetric Procrustes documentation & arguments are finalized.
  2. I have removed References from the docstring because it was making docstring long, and with adding Table 1 (from the paper which contains references) to our documentation, I felt this is redundant. Also, one less thing to keep updated. Let me know what you think @fwmeng88 and @PaulWAyers.

The References will be added in the documentation (in the table
summarizing various Procrustes method).
@PaulWAyers
Copy link
Member

PaulWAyers commented Mar 8, 2021

I'm happy with putting references centrally. I don't worry so much about keeping them up-to-date, or having long documentation (especially if the references are just at the very end anyway), since most of the references we should be including are just "the classics." But I know that because our documentation is embedded in the code, it can be annoying to have a lot of documentation before the code even starts.

I guess the question, then, is whether it is good to have the specific reference where a reader can get more information about the underlying algorithm/method and its justification. I would tend towards including references in the code I guess (where one might wonder what's going on) but as long as there is a source for the references, it is OK I think.

@FanwangM
Copy link
Collaborator

FanwangM commented Mar 8, 2021

We can add a table in About section to summarize all the Procrustes methods (I can take care of it), as we did in the manuscript. But I think adding the references within the codes can help people know exactly what's going on and what are the sources for one specific Procrustes problem.

@FarnazH
Copy link
Member Author

FarnazH commented Mar 9, 2021

Only 4 of the Procrustes functions had "References" and their formats were not consistent, so I thought it's best to remove them. We can add them later when the summary table is added and our list of references is finalized. We should use the same format for adding references to the docstrings. I will make an issue so we wouldn't forget. Is that fine with you?

@FanwangM FanwangM merged commit d7801cf into theochem:master Mar 9, 2021
@FanwangM
Copy link
Collaborator

FanwangM commented Mar 9, 2021

The reference is not a big problem and I have merged this PR.
Many thanks for making the package much nicer!
@FarnazH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants