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

Modified parser to recognise a comma delimited lvm format #14

Closed
wants to merge 4 commits into from

Conversation

Professor-0
Copy link

No description provided.

@jankoslavic
Copy link
Contributor

thank you @Professor-0 ! this is quite a lot of changes for one PR. Please provide background information on the problem this PR is solving?

@Professor-0
Copy link
Author

Sorry, I originally meant to do the PR on my fork. One of my friends who's studying mechanical engineering was given an lvm file to analyse and was directed to this python module to assist. However the module only supported lvm files that were tab delimited and the ones my friend was given were comma delimited. So I rewrote the parser to cope with different Separator headers. It does produce a different signature. The original parser split the data into segments. I couldn't find any examples of an lvm file with multiple data segments so I removed that and just added a singular 'data' key on the output.

@Professor-0
Copy link
Author

Currently it's just a quick fix to get it working for comma lvms, I will go through the lvm format later and push a more robust version that fits the specification

@jankoslavic
Copy link
Contributor

dear @Professor-0, this should be an easy job. The separator is defined in the header. If you provide a short test file, I can try to help.

@Professor-0
Copy link
Author

Hi @jankoslavic, I've written a specification conformant version of the parser. It should be fully functional (besides special blocks).

@jankoslavic
Copy link
Contributor

Hi @Professor-0 thank you for your effort. This PR does not look good to me; way too many changes in one PR. You would like to add comma delimiter, is this true? This is not implemented yet, but it should be a few lines of additional code.
Can you upload please your minimal test file with comma delimiter?

@Professor-0
Copy link
Author

Hi @jankoslavic I've attached a test file: pressure_data_min.txt. I understand if you do not wish to proceed with a specification driven approach. I will close the PR here.

@jankoslavic
Copy link
Contributor

Thank you @Professor-0 for the test file. Here is a much simpler implementation:
#15

Please check it out.

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.

None yet

2 participants