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

NPI-3653 - Update code to handle new V2 IGS logs #60

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

Conversation

ronaldmaj
Copy link
Collaborator

A quick PR to update the code to handle new metadata fields in IGS log files.

This will still be able to read and process the old IGS log files as well (backwards compatible)

@seballgeyer seballgeyer self-requested a review December 12, 2024 23:11
gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
@@ -109,21 +134,25 @@ def parse_igs_log(filename_array: _np.ndarray) -> _np.ndarray:
with open(file_path, "rb") as file:
data = file.read()

blk_id = _REGEX_ID.search(data)
blk_id = _REGEX_ID_V2.search(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

an unit test can be good so we can have a quick check if it works or not?

gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
@seballgeyer seballgeyer self-assigned this Dec 18, 2024
elif result_v2 is not None:
return "v2.0"
else:
return "Unknown"
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment only/suggestion.
I would probably raise in exception if it is Unkown

Suggested change
return "Unknown"
class LogVersionError(Exeption):
"""Custom exception raised when an uknown log version is encountered."""
pass
... in the code ...
else:
raise LogVersionError("Unknown log version")
return "Unknown"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@ronaldmaj ronaldmaj left a comment

Choose a reason for hiding this comment

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

I think I've address the outstanding comments you've left on this. Let me know if I've missed anything

gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
elif result_v2 is not None:
return "v2.0"
else:
return "Unknown"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Show resolved Hide resolved
gnssanalysis/gn_io/igslog.py Outdated Show resolved Hide resolved
try:
version = determine_log_version(data)
except LogVersionError as e:
logger.warning(f"Error: {e}, skipping parsing the log file")
Copy link
Collaborator

Choose a reason for hiding this comment

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

in this case, when the exception is raised, the code will still continue even if the version is uknown,
I am not sure what do we want to do.
one option is to stop the function (either re- raise

except LogVersionError as e:
    logger.warning(...)
   raise

or just an empty return

except .....
     logger.warning(....)
    return

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