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

Export krr, fix tests #7

Merged
merged 6 commits into from
Jan 31, 2022

Conversation

JeffFessler
Copy link
Member

and more use of using to help reader (e.g., me) know what is used where.

@JeffFessler
Copy link
Member Author

In cases where the error in v1.7 was similar, I just replaced with with the new error.
In cases where the new error was noticeably bigger, I left the old error in as a comment in case you want to check.
The test values work with v1.7 (not even v1.6, to my surprise) so I bumped the julia version to 1.7.

@StevenWhitaker
Copy link
Collaborator

I had wanted to incorporate StableRNGs.jl into the tests, so thanks for making a PR to motivate me to do that :)

I'll just merge this PR after CI finishes.

@StevenWhitaker
Copy link
Collaborator

Hmm, tests pass locally but not in CI...

@JeffFessler
Copy link
Member Author

Having to put rng as an arg in many places seems inconvenient for user and maybe is a lot of breaking changes.
Using a named kwarg with a sensible default ; rng = Random.GLOBAL_RNG would be more appealing to me.

The values I put in there are for julia 1.7 so I don't know if they work with 1.6.
But why "Julia 1" fails is baffling.

@StevenWhitaker
Copy link
Collaborator

I agree; rng = Random.GLOBAL_RNG is the default (just not a kwarg). (I try to avoid kwargs because they don't participate in dispatch, so I think it at least slightly affects performance.) So there shouldn't be any breaking changes.

I should have clarified: when I added StableRNGs, I also changes all the comparison values (overwriting the changes you made). So the tests pass locally for me for both v1.6.5 and v1.7.1, so I'm not sure why they fail on CI.

@StevenWhitaker
Copy link
Collaborator

I'm just going to merge this and then try to figure out the CI issues separately.

@StevenWhitaker StevenWhitaker merged commit 52d220b into MagneticResonanceImaging:main Jan 31, 2022
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

2 participants