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 argument for spline boundary conditions #2114

Merged
merged 7 commits into from Mar 15, 2023

Conversation

awkwardPotato812
Copy link
Contributor

@awkwardPotato812 awkwardPotato812 commented Mar 9, 2023

Checklist
Thank you for contributing to QuTiP! Please make sure you have finished the following tasks before opening the PR.

  • Please read Contributing to QuTiP Development
  • Contributions to qutip should follow the pep8 style.
    You can use pycodestyle to check your code automatically
  • Please add tests to cover your changes if applicable.
  • If the behavior of the code has changed or new feature has been added, please also update the documentation in the doc folder, and the notebook. Feel free to ask if you are not sure.
  • Include the changelog in a file named: doc/changes/<PR number>.<type> 'type' can be one of the following: feature, bugfix, doc, removal, misc, or deprecation (see here for more information).

Description
Added an argument bc_type to allow users to specify boundary conditions for interpolating QobjEvo

Related issues or PRs
fix #2098

qutip/core/coefficient.py Outdated Show resolved Hide resolved
qutip/core/coefficient.py Outdated Show resolved Hide resolved
@hodgestar
Copy link
Contributor

@awkwardPotato812 Thanks for starting this PR. I did a partial review and activated the test run. I will leave a full review for Eric to do though.

@coveralls
Copy link

coveralls commented Mar 9, 2023

Coverage Status

Coverage: 75.43% (+3.3%) from 72.089% when pulling e701ecc on awkwardPotato812:master into 40ff8e1 on qutip:master.

Co-authored-by: Simon Cross <hodgestar+github@gmail.com>
@awkwardPotato812
Copy link
Contributor Author

I'm changing this draft to the complete PR. I'll update it with any changes related to documentation once the build error is resolved.

@awkwardPotato812 awkwardPotato812 marked this pull request as ready for review March 10, 2023 06:41
Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach is good, thank you.
But the variable name should be fixed either boundary_conditions or bc_type. As Simon said, bc_type can be confusing for those not familiar with scipy functions, so let's use boundary_conditions everywhere.

qutip/core/coefficient.py Outdated Show resolved Hide resolved
qutip/core/cy/coefficient.pyx Outdated Show resolved Hide resolved
Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close.

qutip/core/cy/coefficient.pyx Outdated Show resolved Hide resolved
qutip/core/cy/qobjevo.pyx Outdated Show resolved Hide resolved
qutip/tests/core/test_coefficient.py Outdated Show resolved Hide resolved
@Ericgig
Copy link
Member

Ericgig commented Mar 14, 2023

The new test is failing.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good, thank you!

@Ericgig Ericgig merged commit 8ae215a into qutip:master Mar 15, 2023
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

4 participants