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

Added G18 data loading for pipeline database data #23

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

jcsmithhere
Copy link
Collaborator

Very simple change but creating pull request since @anthonyozerov is the maintainer of this code and will know better than me if this simple change will cause problems.

@jcsmithhere
Copy link
Collaborator Author

Pinging this Pull Request. @anthonyozerov, as the Maintainer of this codebase, I would like you to agree to all pull requests before merging. This is a very simple change, nonetheless, I'd like some procedure to maintain this codebase. Nominally, you would do the merge to main.

@anthonyozerov
Copy link
Collaborator

I think this won't be compatible with database files made by past pipeline versions, as they won't have the G18 attribute in the stereoFeatures object. If we care about this we can just add a:

if 'G18' in stereoFeatures:…

This would help if anyone wants to load some older pipeline data that's floating around. But maybe we don't want to support that?

@jcsmithhere
Copy link
Collaborator Author

Yes, legacy support is an important thing to consider. I don't think we have the resources to support all previous versions of the database, and the database is still in development. We could sync the bolides package with each pipeline release then check for compatibility.

Will this change break your data from last summer? I think I didn't have the G18 code in until the end of 2022. Being able to continue to use the data processed last summer is probably good, but it might be too much work to allow legacy data before that.

I'll make the change to add the conditional you have above.

@anthonyozerov
Copy link
Collaborator

Yes it will break the data from last summer, as that older database file doesn't have G18. So doing the conditional would be good to maintain compatibility with that data.

@jcsmithhere
Copy link
Collaborator Author

I checked and the databases from the summer of 2022 would not load with this change. So I placed the change in a conditional to allow for backwards compatibility. Tested changes on both a new database and an old one. I will assume Anthony has no other comments and will go ahead and merge.

@jcsmithhere jcsmithhere merged commit 5c07db7 into master Jun 29, 2023
@jcsmithhere
Copy link
Collaborator Author

Merged to master. Closing PR.

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