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

added info about sec_cutoff to the documentation #2136

Merged
merged 11 commits into from Apr 11, 2023
Merged

Conversation

gsuarezr
Copy link
Contributor

@gsuarezr gsuarezr commented Mar 27, 2023

Description
I added a note at the end of the time independent Bloch-Redfield equation section, to indicate that one can simulate the nonsecular version using the sec_cutoff keyword

@Ericgig
Copy link
Member

Ericgig commented Mar 27, 2023

Could you fix the git diff issue. We can't see your changes like this.

@gsuarezr
Copy link
Contributor Author

Hi Eric, I would love to but I simply don't know what the git diff issue is, might it be that only the file that I modified should show in the diff?

@Ericgig
Copy link
Member

Ericgig commented Mar 27, 2023

Yes, only the files you modified should appear in the diff, right now it seems like your PR changed 256 files, but the files seems to have the same content... If we can't easily see what changed, we can't review the PR.
I am not sure how this happen or how to fix it...

@gsuarezr
Copy link
Contributor Author

Thanks for clearing that, I'm also not sure how that happened, but it should be fixed now

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.

Thank you for the contribution.

The example should be in a code block.
Could you fix format (2 spaces between words, missing capitalization, ...).

@coveralls
Copy link

coveralls commented Mar 29, 2023

Coverage Status

Coverage: 75.317% (-0.01%) from 75.328% when pulling 4d194ae on mcditoos:gsocApp into ab6a0d7 on qutip:master.

@gsuarezr
Copy link
Contributor Author

Thanks for your feedback, I think it should be better now

@Ericgig
Copy link
Member

Ericgig commented Apr 3, 2023

@nwlambert could you confirm that the addition is exact?
I am not sure that "partial secular approximation" is a good explanation.

@nwlambert
Copy link
Member

seems ok, perhaps just referencing how sec_cutoff relates to the condition mentioned above equation 4 in the documentation would make it more precise?

@gsuarezr
Copy link
Contributor Author

gsuarezr commented Apr 6, 2023

Hi, I added the reference to the equation. I wanted to be more specific but couldn't figure out from the code if the cutoff is something like $|w_{ac}-w_{cd}| \lt cutoff w_{0}$ or without $w_{0}$

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.

Let's just fix the spacing in the code and it will be ready to merge.

Co-authored-by: Eric Giguère <eric.giguere@calculquebec.ca>
@gsuarezr
Copy link
Contributor Author

Sorry for taking this long to reply, I was on holiday. Should be ready now, thanks !

@Ericgig Ericgig merged commit 4ffea33 into qutip:master Apr 11, 2023
12 checks passed
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