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

Update to allow NaNs for IEEE754 data types #646

Merged
merged 5 commits into from
May 19, 2023
Merged

Update to allow NaNs for IEEE754 data types #646

merged 5 commits into from
May 19, 2023

Conversation

al-niessner
Copy link
Contributor

@al-niessner al-niessner commented May 15, 2023

🗒️ Summary

Updated the field check type check to allow nan and inf in table real IEEE types -- see comments on #514 and below for more details on nan and inf handling with IEEE types.

⚙️ Test Data and/or Report

See unit tests

♻️ Related Issues

Closes #644

Al Niessner added 2 commits May 15, 2023 15:10
However the test data seems to be a problem because it is failing on an offset which seems to be unrelated to the reported problem.
@al-niessner al-niessner self-assigned this May 15, 2023
@al-niessner al-niessner marked this pull request as draft May 15, 2023 22:14
@al-niessner
Copy link
Contributor Author

@jordanpadams

This issue and solution are contradictory to #514 which does not want a NaN in a binary file. Because of this, the unit test for 644 passes while 514 fails. There is no resolution for this other to discover which is the correctly bug.

@jordanpadams
Copy link
Member

@rchenatjpl can you confirm that the test data from #514 may be incorrect? at least looking at the test data on this comment, we coded up validate to throw errors on 4table.xml, but it appears those values are all IEEE:

    <Table_Binary>
      <offset unit="byte">0</offset>
      <records>4</records>
      <description>FITS EDU number:  6
          CAL_T1  = 0.3243 / Deposited TOF = T0 + T1*Raw TOF</description>
      <Record_Binary>
        <fields>2</fields>
        <groups>0</groups>
        <record_length unit="byte">8</record_length>
        <Field_Binary>
          <name>ET</name>
          <field_number>1</field_number>
          <field_location unit="byte">1</field_location>
          <data_type>IEEE754MSBSingle</data_type>
          <field_length unit="byte">4</field_length>
          <unit>Seconds</unit>
          <description>Ephemeris Time 12:00:00 UTC</description>
        </Field_Binary>
        <Field_Binary>
          <name>YEAR</name>
          <field_number>2</field_number>
          <field_location unit="byte">5</field_location>
          <data_type>IEEE754MSBSingle</data_type>
          <field_length unit="byte">4</field_length>
          <unit>Years</unit>
          <description>Fractional Year (UTC). Use ET for accuracy!</description>
        </Field_Binary>
      </Record_Binary>
    </Table_Binary>

actually same goes for the next comment and associated test data referred to by 8table.xml:

    <Table_Binary>
      <offset unit="byte">0</offset>
      <records>4</records>
      <description>FITS EDU number:  6
          CAL_T1  = 0.3243 / Deposited TOF = T0 + T1*Raw TOF</description>
      <Record_Binary>
        <fields>1</fields>
        <groups>0</groups>
        <record_length unit="byte">8</record_length>
        <Field_Binary>
          <name>DAY_OF_YEAR</name>
          <field_number>1</field_number>
          <field_location unit="byte">1</field_location>
          <data_type>IEEE754MSBDouble</data_type>
          <field_length unit="byte">8</field_length>
          <unit>Days</unit>
          <description>Fractional Day of Year (UTC).  Use ET for accuracy!</description>
        </Field_Binary>
      </Record_Binary>
    </Table_Binary>

if we change those field definitions to be ASCII_Real would that make those test cases valid?

@al-niessner al-niessner marked this pull request as ready for review May 18, 2023 15:59
@jordanpadams jordanpadams changed the title Issue 644: NaN being reported for IEEE754 types Update to allow NaNs for IEEE754 data types May 19, 2023
@jordanpadams jordanpadams merged commit d91dfe8 into main May 19, 2023
@jordanpadams jordanpadams deleted the issue_644 branch May 19, 2023 19:50
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.

Validate gives errors for 'NaN' and 'Inf' values in IEEE754 data
2 participants