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

Add Solver options and list them in API doc #2163

Closed
4 tasks
AGaliciaMartinez opened this issue May 17, 2023 · 9 comments · Fixed by #2167
Closed
4 tasks

Add Solver options and list them in API doc #2163

AGaliciaMartinez opened this issue May 17, 2023 · 9 comments · Fixed by #2167
Assignees
Labels

Comments

@AGaliciaMartinez
Copy link
Member

Problem Description

The options that can be passed to the solvers are not entirely clear by looking at the documentation. For example:
image
Does not show the explicit options even thought it refers to them in the parameter documentation. For comparison, SESolver does show the options.

This I assume is because the options are inherited from SESolver. But this is not clear either by looking at MESolver's rendered documentation.
image

When using help(MESolver) the documentation of the class is much clearer but perhaps too verbose for the online documentation.

Proposed Solution

I would suggest we render the options property explicitly for each solver. I would also consider rendering the documentation related to the run method. If not, we should render at least Solver documentation which does not seem to be listed in the class API documentation for solvers.

Affected solvers:

  • MESolver
  • SMESolver
  • SSESolver
  • NonMarkovianMCSolver

Alternate Solutions

No response

Additional Context

The documentation was taken from qutip 5.0 "ReadTheDocs".

@hodgestar
Copy link
Contributor

Plus one on adding the options and run methods to the list of methods rendered in the documentation.

@nathanshammah nathanshammah changed the title Solver options Add Solver options and list them in API doc May 25, 2023
@EmilianoG-byte
Copy link
Contributor

Hi! If I want to address this issue, should I just fork the repo, create my own branch an open a merge request?

@AGaliciaMartinez
Copy link
Member Author

Yes, go ahead, we are looking forward to your contribution! 😄

@EmilianoG-byte
Copy link
Contributor

I have just created the PR, I hope this solves the issue as expected. If not I am happy to discuss there 😃

@EmilianoG-byte
Copy link
Contributor

If the issue is done but not merged yet, do I get assigned anyways so it can be crossed out from the unitary hack page and I get my bounty? 🤔

@AGaliciaMartinez
Copy link
Member Author

Merged now 😄 . Thanks for the changes!

@EmilianoG-byte
Copy link
Contributor

Awesome! If you have time to answer the final comments I had in the PR it would be really helpful 😀

Also, I think I need to be added as assignee for the bounty to be closed in the unitary hack.

@BoxiLi
Copy link
Member

BoxiLi commented Jun 2, 2023

Done @EmilianoG-byte

@nathanshammah
Copy link
Member

Congrats @EmilianoG-byte!

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

Successfully merging a pull request may close this issue.

5 participants