Skip to content

BUG: setting Data_version in write_cdf #1738

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

Merged
merged 2 commits into from
May 27, 2025

Conversation

tech3371
Copy link
Contributor

Change Summary

Overview

small change to fix Data_version=None throwing error on loading CDF file.

Updated Files

  • imap_processing/cdf/utils.py
    • small fix

Copy link
Contributor

@maxinelasp maxinelasp left a comment

Choose a reason for hiding this comment

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

Good catch! Would you also be able to update the CLI and this code to set the version to "001" instead of "v001" as requested by Andriy? Should just be able to chop it to the last thee characters in those two spots.

@greglucas
Copy link
Collaborator

Would you also be able to update the CLI and this code to set the version to "001" instead of "v001" as requested by Andriy? Should just be able to chop it to the last thee characters in those two spots.

Oof :( Do we need to update this in imap-data-access too so we can reference the version everywhere as just 3 numbers and only our filenames would print out v as a prefix in that field?

@tech3371
Copy link
Contributor Author

Would you also be able to update the CLI and this code to set the version to "001" instead of "v001" as requested by Andriy? Should just be able to chop it to the last thee characters in those two spots.

Oof :( Do we need to update this in imap-data-access too so we can reference the version everywhere as just 3 numbers and only our filenames would print out v as a prefix in that field?

@greglucas Can you help me with this work?

@maxinelasp
Copy link
Contributor

@greglucas AFAIK it is just in the attribute within the file, but Veronica brought it up. No way should we change the filename again - they had their chance to say no!

@maxinelasp
Copy link
Contributor

I think imap-data-access and everything else uses the filename, so it shouldn't affect anything except (maybe?) the reading in and writing out of files

@greglucas
Copy link
Collaborator

No way should we change the filename again - they had their chance to say no!

💯 I completely agree :)

I think in all of our ScienceFile.version, Database.version, etc... we are using the "vXXX" format as a string with the v prefix. Is it OK if we have a file.global_attribute["Data_version"] == "001" but the filename would be access as a version with "v001"? I don't think we are really using the Data_version global attribute at all ourselves, just putting that into the file, so perhaps we just go with the minimal update @maxinelasp suggests and don't worry about our internal versioning because that is consistent on our end already?

@maxinelasp
Copy link
Contributor

@greglucas yes that was exactly what I was thinking, just change the attribute. It wasn't even added into files consistently until recently so I don't think it's used anywhere.

tech3371 and others added 2 commits May 27, 2025 11:51
SPDF does not want vXXX in their attribute, they would prefer XXX.
This updates the Data_version attribute, but still puts the vXXX in
the filename.
@greglucas
Copy link
Collaborator

@tech3371, I just pushed a commit that updates the Data_version attribute for everyone automatically and updates all of the corresponding tests that expect that value. Filenames still have "vXXX", but the attribute only has "XXX" now.

@tech3371
Copy link
Contributor Author

@tech3371, I just pushed a commit that updates the Data_version attribute for everyone automatically and updates all of the corresponding tests that expect that value. Filenames still have "vXXX", but the attribute only has "XXX" now.

Thank you @greglucas. Changes looks good to me. Would you like me to merge it?

@greglucas
Copy link
Collaborator

Thank you @greglucas. Changes looks good to me. Would you like me to merge it?

Yes, if you are OK with those changes then please merge.

@tech3371 tech3371 merged commit 2870eb1 into IMAP-Science-Operations-Center:dev May 27, 2025
14 checks passed
@bourque bourque added this to IMAP May 29, 2025
@bourque bourque added this to the May 2025 milestone May 29, 2025
@bourque bourque moved this to Done in IMAP May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants