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

Validate does not handle Special_Constants valid_minimum and valid_maximum in accordance with information model #673

Closed
jstone-psi opened this issue Jul 24, 2023 · 2 comments · Fixed by #678
Assignees
Labels
B14.0 bug Something isn't working s.medium

Comments

@jstone-psi
Copy link

Checked for duplicates

Yes - I've already checked

🐛 Describe the bug

Running validate on labels with valid_minimum and valid_maximum specified results in validation errors describing values are beyond the valid_minimum and valid_maximum. However, the information model states that these values are not errors, but instead have a "special meaning". That would indicate that these validation errors are incorrect.

🕵️ Expected behavior

I would expect values that are beyond the valid_minimum and valid maximum to not give an error, since they are allowed to appear in the data. There might be an argument for raising a warning if the data values are beyond are beyond the valid range, however, even this is problematic, since the current validation is checking the stored value. The definitions for valid_minumum and valid_maximum do not specify whether they apply to the stored or scaled value, so this would require the attention of DDWG to fix.

I previously raised an issue about minimum and maximum values which was related to Object_Statistics, and not special constants. This seems like the appropriate place to have this validation. It would be a good idea to make the distinction between the min/max values in Object_Statistics and Special_Constants clearer, perhaps in the DPH.

#434

📜 To Reproduce

Run validate on a label with valid_minimum and valid_maximum supplied, and a data file that contains values beyond these ranges (again, this is up for interpretation)

Sample data attached:

grf99034.xml.zip
grf99034.fit.zip

Validator output:

grf99034.txt

🖥 Environment Info

  • Version of this software validate 3.2.0
  • Operating System: MacOS 13.4.1
    ...

📚 Version of Software Used

gov.nasa.pds:validate
Version 3.2.0
Release Date: 2023-04-14 00:53:23

Copyright 2019, by the California Institute of Technology ("Caltech").
All rights reserved.

🩺 Test Data / Additional context

The definition of valid_maximum

https://pds.nasa.gov/datastandards/documents/im/v1/index_1K00.html#attribute_pds_special_constants_pds_valid_maximum

This was mentioned in the discussion for the original issue #611, however, is was after the implemenation of this feature, and should be elevated to a full issue.

#611 (comment)
#611 (comment)

🦄 Related requirements

🦄 #611

⚙️ Engineering Details

No response

@jstone-psi jstone-psi added bug Something isn't working needs:triage labels Jul 24, 2023
@al-niessner
Copy link
Contributor

@jordanpadams

This one is up to you. I can dig up the old ticket if necessary, but you specifically had me add them as an error for another user. Let me know if the code needs to go back to a warning.

@jordanpadams
Copy link
Member

@al-niessner apologies. that was my mistake, let's just switch these two back to warnings. Sorry!

@github-project-automation github-project-automation bot moved this to Release Backlog in B14.0 Aug 4, 2023
@jordanpadams jordanpadams moved this from Release Backlog to 🚀 Sprint Backlog in B14.0 Aug 4, 2023
jordanpadams added a commit that referenced this issue Aug 8, 2023
#673: change special constant min/max excursion error to warning
@github-project-automation github-project-automation bot moved this from 🚀 Sprint Backlog to 🏁 Done in B14.0 Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B14.0 bug Something isn't working s.medium
Projects
No open projects
Status: 🏁 Done
Development

Successfully merging a pull request may close this issue.

3 participants