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

Gb edf metadata adapter #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

rajasriramoju
Copy link
Contributor

This branch deals with the adapter for .gb files. Additional functionality has been added for .gb file ingestion. The goal is to take metadata associated with the .edf images associated to the respective sfloat .gb image: the hi and lo .edf images. The helper function create new indices if the same key exists with different values in the hi and low images' metadata, in order to not lose information.

The current version works in ingesting .gb files when there are .edf files with the associated .txt files. But there is a bug when ingesting a folder with just .gb files. This adapter needs debugging. The current terminal output looks like this:

Screenshot 2024-12-04 at 8 01 41 PM

with a final error statement of: AttributeError: 'NoneType' object has no attribute 'new'

@rajasriramoju rajasriramoju requested a review from Wiebke December 4, 2024 16:02
…t existing .edf files in the directory (user copied only relevant files)
Copy link
Member

@Wiebke Wiebke left a comment

Choose a reason for hiding this comment

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

Overall looks great.

I left a few very minor comments, the one regarding date comparison is likely of more relevance for the similar if a simple sort mechanism on strings is in use there currently.


# Compare two dates and select the later one
if hi_date is not None and lo_date is not None:
gb_dictionary["Date"] = hi_date if hi_date > lo_date else lo_date
Copy link
Member

Choose a reason for hiding this comment

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

At this point, both hi_date and lo_date should be of type string. Given that the format in the header is of form e.g. Sat Apr 1 11:35:48 2023 this may be lead to incorrect ordering if high and low scan happen to involve a date switch.

This is unlikely to matter much here, but would be important to take into account in e.g. sorting according to date.

Comment on lines +127 to +128
lo_header = lo_file.header
lo_date = lo_header.get("Date")
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason why the remainder of the header information is not added to the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The header information will now be added to the metadata dictionary. Previously, only the date was required from the header, but the header data will not be added to avoid loss of any information.

Comment on lines 120 to 121
# File does not exist, set the date as None - empty metadata dictionary is
# returned in the parse_txt_accompanying_edf method
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a slightly outdated comment.

Copy link
Contributor Author

@rajasriramoju rajasriramoju Jan 17, 2025

Choose a reason for hiding this comment

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

The comment isn't outdated, but it will be written in a better way to explain why an empty dictionary is needed.

@rajasriramoju rajasriramoju requested a review from Wiebke January 23, 2025 08:03
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.

2 participants