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

add floating point support to cider #2335

Merged
merged 3 commits into from
Nov 6, 2024
Merged

add floating point support to cider #2335

merged 3 commits into from
Nov 6, 2024

Conversation

ekiwi
Copy link
Contributor

@ekiwi ekiwi commented Nov 5, 2024

Somewhere, things still get messed up.

I get:

{
  "out": [
    15.123900413513184,
    0.5600000023841858
  ],
  "inp": [
    15.123900413513184
  ]
}

But the expected output is:

{
  "inp": [
    15.12
  ],
  "out": [
    15.12,
    0.56
  ]
}

@EclecticGriffin
Copy link
Collaborator

Oh yeah this is the printing problem I mentioned

@ekiwi ekiwi marked this pull request as ready for review November 5, 2024 20:20
@ekiwi
Copy link
Contributor Author

ekiwi commented Nov 5, 2024

Looks like I might have been able to solve the issue with the use of Number::from_string_unchecked(value) which does not roundtrip the number through the parser.

@ekiwi
Copy link
Contributor Author

ekiwi commented Nov 5, 2024

New output:

{
  "out": [
    15.1239,
    0.56
  ],
  "inp": [
    15.1239
  ]
}

@rachitnigam
Copy link
Contributor

Woo! Awesome! Thanks for taking this up @ekiwi!

Copy link
Collaborator

@EclecticGriffin EclecticGriffin left a comment

Choose a reason for hiding this comment

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

Looks great! Had a very minor question about a signature, but otherwise I think it's good to go

tools/cider-data-converter/src/converter.rs Show resolved Hide resolved
} => {
let value = match width {
32 => {
debug_assert_eq!(chunk.len(), 4);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make these just assert!? The test probably isn't expensive here

Comment on lines +307 to +308
// we need to inject the string directly in order to maintain the correct rounding
Number::from_string_unchecked(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Truly cursed! Totally makes sense, though

@@ -227,7 +239,7 @@ impl ParseVec {
}

pub fn parse(&self, format: &FormatInfo) -> Result<DataVec, ParseError> {
if format.is_fixedpt() {
if format.is_fixedpt() || format.is_floating_point() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Glad we could reuse all this!

@ekiwi
Copy link
Contributor Author

ekiwi commented Nov 5, 2024

Now this fails because the test is wrong: #2336

@ekiwi
Copy link
Contributor Author

ekiwi commented Nov 5, 2024

@rachitnigam : now the ball is in your court: #2336

@rachitnigam
Copy link
Contributor

#2337 but still has a roundtripping bug. We should also fix support for negative floating-point values while we're at it.

--through cider \
-s sim.data={}.data \
-s calyx.args="--log off" \
{} | jq --sort-keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the new jq stage.

ekiwi pushed a commit that referenced this pull request Nov 6, 2024
Fixes #2336. Currently running into the same problem as
#2335 (comment)

@ekiwi how did you fix it on the Rust side?
@ekiwi ekiwi merged commit 42880f1 into calyxir:main Nov 6, 2024
9 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.

3 participants