-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Define an API surface between Spezi Data Pipeline and Python notebook #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vicbi Thank you for the PR and first version of the decomposed architecture; it is great to see some next steps and to see the architecture and elements that we discussed taking shape within the PR!
I started to dive into the code but came up with a lot of basic code style feedbacks so I decided to fist install a linter in a GitHub action and I added a proposed linting file to define a common code style to agree on.
I think this would be a great next step. Let me know what you think about this configuration. Feel free to adapt it and incorporate the elements that it provides as feedback into this PR so we have a well-formatted codebase to start with.
In addition to that, I have also provided some high-level feedback on some functions and elements; I can provide some more aspects once we have an agreement about the code style guidelines 🚀
…PipelineTemplate into defineAPISurface
Hi @PSchmiedmayer, I've pushed the code updates to the defineAPISurface branch. I chose to keep the ECGObservation class (potentially, it can be a wrapper for the Observation class). This approach simplifies method selection across modules based on data type, eliminating the need for conditions to exclude ECG LOINC code from the FHIRDataFrame object. Moving forward, we might consider renaming ECGObservation to cover all LOINC codes with attributes similar to ECG data. Thank you for reviewing the changes. Please note that the README hasn't been updated to include these adjustments yet, but it will be shortly. However, the docstrings within the modules are up-to-date. Feel free to reach out if you need any help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work here @Vicbi; happy to see this merged soon! 🚀
I took a look at the code and would have a few high-level suggestions
- The ECG parsing is currently assuming a very rigid structure of the elements ordered in the correct way. We can't really guarantee the order in the JSON files and should therefore rather use the coding schemes attached to the elements to parse this.
- Unit Testing CI Setup: We currently only execute the notebook for the CI setup but not any of the unit test as far as I can see. It would be good if they are executed as part of the GitHub Action CI setup.
- Related to that: Test Coverage. Thank you for writing some first tests. I think it would be important to automatically asses what lines are already covered and where we are missing tests to ensure that the code is working as expected. We currently use CodeCov (https://about.codecov.io) for all our projects and I would suggest to use it here as well to generate automated coverage reports. They have some good documentation for python code.
- Code Linting: The current lint rules still throw some errors that would be great to be addressed before we merge the PR.
- As discussed, we will also need to adjust the README to reflect the latest changes.
Apart from this: I would suggest to merge the PR without too many smaller changes to ensure that we have a reworked state in the main branch and we can use subsequent smaller PRs and our meetings to discuss smaller changes to refine the API.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🎉
Define an API surface between Spezi Data Pipeline and Python notebook
♻️ Current situation & Problem
This PR addresses issue #7. The current helper.py file lacks structure and does not offer a clear API surface.
⚙️ Release Notes
📚 Documentation
API Surface Structure
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: