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

fix: ReadOnlyDirectory should provide the largest abs(cycle) when cycle is unspecified, not the largest cycle. #715

Merged
merged 3 commits into from Sep 20, 2022

Conversation

jpivarski
Copy link
Member

No description provided.

@jpivarski jpivarski linked an issue Sep 19, 2022 that may be closed by this pull request
@jpivarski
Copy link
Member Author

@agoose77, you've already weighed in on what you think in the issue comments, but I'm going to always ask for reviews from now on, even if they're easy. If you agree with this fix, you can also merge it. (What I'm saying here is that I won't be making any more changes, which is what "request to review" is supposed to mean.)

@jpivarski
Copy link
Member Author

I just remembered that I'll need to make a corresponding v4 version of this PR. I'll do that soon.

@jpivarski
Copy link
Member Author

This passed tests, but I think it was because it ran before the boost-histogram update. Therefore, we'll do the same procedure:

  1. test: adjust for boost-histogram 1.3.2's _storage_type deprecation. #719 gets approved and merged into main.
  2. This PR updates to main.
  3. The tests re-run and pass (again).
  4. This PR gets approved and merged into main.

@agoose77
Copy link
Collaborator

If #721 gets merged, I'm happy with this being merged.

@jpivarski
Copy link
Member Author

I'll take that as an approval of both #721 and #715 and enable auto-merge.

@jpivarski jpivarski enabled auto-merge (squash) September 20, 2022 16:38
@jpivarski jpivarski merged commit fd918e6 into main Sep 20, 2022
@jpivarski jpivarski deleted the jpivarski/default-to-latest-abs-fCycle branch September 20, 2022 20:25
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.

Which cycle ReadOnlyDirectory should return when not specified
2 participants