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.

Sign up for GitHub

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

Jump to bottom

Force statistical inefficiency computation to use self.max_n_iterations #577

Merged
merged 3 commits into from
Apr 25, 2022
Merged

Force statistical inefficiency computation to use self.max_n_iterations #577

merged 3 commits into from
Apr 25, 2022

Conversation

zhang-ivy
Copy link
Contributor

Description

I noticed that in MultiStateAnalyzer.generate_mixing_statistics(), self.max_n_iterations is used to generate the transition matrix and compute the perron eigenvalue, but it's not used to compute the statistical inefficiency. This PR supplies self.max_n_iterations to the statistical inefficiency computation, so that the whole function is consistent (in terms of iterations used) in the mixing statistics it outputs.

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

  • Ready to go

@zhang-ivy zhang-ivy requested review from ijpulidos and jchodera April 18, 2022 16:18
@zhang-ivy zhang-ivy changed the title use self.max_n_iterations Force statistical inefficiency to use self.max_n_iterations Apr 18, 2022
@zhang-ivy zhang-ivy changed the title Force statistical inefficiency to use self.max_n_iterations Force statistical inefficiency computation to use self.max_n_iterations Apr 18, 2022
ijpulidos
ijpulidos reviewed Apr 18, 2022
View reviewed changes
Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks! I still would like @jchodera to take a look into this one.

@zhang-ivy
Copy link
Contributor Author

@ijpulidos @jchodera : Just pushed a commit addressing John's concerns from our perses dev sync today. states is in fact a 2D object, but since we are slicing the first axis, the comma is not necessary. I added it anyway, for clarity.

jchodera
jchodera approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

Thanks for catching and fixing this!

@jchodera jchodera added this to the 0.21.4 milestone Apr 20, 2022
@jchodera
Copy link
Member

@zhang-ivy : Can you update the release notes too? You'll have to create a new 0.21.4 entry.

@zhang-ivy
Copy link
Contributor Author

Done! @ijpulidos could you review again and merge?

ijpulidos
ijpulidos approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@ijpulidos ijpulidos left a comment

Choose a reason for hiding this comment

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

LGTM!

@ijpulidos ijpulidos merged commit 1402532 into main Apr 25, 2022
@ijpulidos ijpulidos deleted the fix-stat-ineff branch April 25, 2022 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchodera jchodera jchodera approved these changes

@ijpulidos ijpulidos ijpulidos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21.4
Development

Successfully merging this pull request may close these issues.
3 participants
@zhang-ivy @jchodera @ijpulidos