Skip to content

Conversation

@pjaap
Copy link

@pjaap pjaap commented Apr 29, 2025

This resolves #79

This enables the missing stochastic rounding calls on both deterministic and stochastic input.

@pjaap
Copy link
Author

pjaap commented Apr 29, 2025

@milankl Thanks for this nice package. I am researching in the area of numerical analysis with stochastic floats and kind of need this small presented feature :)

I think it just needs a change in the few lines. Test are fine locally. Why are the tests not triggered in this PR? I can have a look if you want :)

edit: Workflow needs approval. This just popped up.

@milankl
Copy link
Owner

milankl commented Apr 29, 2025

Awesome, thank you!! Will review after CI passes 🥳

@milankl
Copy link
Owner

milankl commented Apr 29, 2025

Would you mind adding some tests to verify that this is actually the case?

  • deterministic -> deterministic is deterministically rounded
  • stochastic -> deterministic is deterministically rounded
  • deter -> stoch is stoch rounded
  • stoch -> stoch also stoch rounded

@pjaap
Copy link
Author

pjaap commented May 5, 2025

Sorry for the late answer. I will add tests and investigate the failure on 1.11.
I am busy this week, so it may take a few days.

@pjaap
Copy link
Author

pjaap commented May 7, 2025

Testing is not so trivial with non-deterministic values :)

I think

deterministic -> deterministic is deterministically rounded

is not in the scope of this package.

stochastic -> deterministic is deterministically rounded

this is already done in the tests. I adjusted the name of the testsets.

deter -> stoch is stoch rounded

this is now implemented. I check 10000 times that the result differs at least once for 100 random numbers :)

stoch -> stoch also stoch rounded

I am not sure what to test. The same test as above with Float64sr?

@pjaap pjaap force-pushed the feature/round-on-conversion branch from b222653 to 717adaf Compare May 7, 2025 16:10
end
end

@test is_stochastic == true

Choose a reason for hiding this comment

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

No need to check that a boolean is equal to another boolean, you can just test its boolean value 🙂

Suggested change
@test is_stochastic == true
@test is_stochastic

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I know. I found it more readable as a user in this particular scenario.
A failing test is more semantic since we get the message

  Expression: is_stochastic == true
   Evaluated: false == true

instead of

  Expression: is_stochastic

I think it is better in the first variant.

But I can change it to the second one if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

no need, that's fine by me and I agree with your point

@pjaap pjaap force-pushed the feature/round-on-conversion branch from 717adaf to 59929da Compare May 22, 2025 08:41
@milankl
Copy link
Owner

milankl commented Aug 31, 2025

Just saw https://pretalx.com/juliacon-local-paris-2025/talk/UDQLTD/. Very nice, thanks for talking about this!

Sorry I can have a look at this pull request next week and merge. Feel free to ping me in case I forget

@pjaap
Copy link
Author

pjaap commented Sep 8, 2025

Hey there! I was on vacation until today :) so, ping?

I was looking for an interesting topic for the JuliaCon and your package applied to our PDE solvers seemed perfect :)
Luckily, the talk was approved. Looking forward to it.

@milankl
Copy link
Owner

milankl commented Sep 8, 2025

This still fails at v1.11 you mind retriggering the CI?

@pjaap
Copy link
Author

pjaap commented Sep 8, 2025

I added a small commit. But I cannot start or restart CI.

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.

Stochastically rounded conversions

3 participants