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

DSEGOG-289 DSEGOG-291 DSEGOG-292 DSEGOG-293 DSEGOG-290 Implement functions with Lark #86

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

Conversation

patrick-austin
Copy link
Contributor

@patrick-austin patrick-austin commented Jan 31, 2024

New functionality

  • New endpoints:
    • /functions for validating types and variable names of a proposed function (intended to be used duringthe popup, before submitting request to /records
    • /functions/tokens to get the list of supported tokens, "nice" names and implementation details
    • /images/function to generate a single full size image from a function
    • /waveforms/function to return x and y of a waveform generated from a function
  • Altered endpoints:
    • /records accepts functions as an optional query param
  • Tests for new functionality

Other changes

  • Development done against python 3.11, and have altered dependencies to allow this (see TODOs)
  • Use json.dumps instead of str in WaveformModel to properly handle NaNs and Inf values in the response
  • Refactor get_channel_dtype

TODOs

  • Considering refactoring builtin functions (no functional impact). While the current builtins cover everything in the requirements, more might be needed later, and it has been raised that a more "reflexive" approach might be more sustainable. While explicitly defining functions was OK originally (min, max, log etc.) with all the builtins updating all the places they need references can be tedious and easy to miss one. Refactoring onto a single source of truth and using that might be more maintainable in the long run, e.g. having a single function rule in the parser and doing lookup in its Transformer function based on a single dict/class (to be created).
    • Implemeted this change, all builtin functions are now defined statically within a class extending the ABC Builtin. All evaluation functionality is defined here, with shared functionality defined on Builtin. Properties are used to specify accepted types and tokens for the frontend. Once a new class is added, it will need to be added to the dict on Builtins but other than that no changes to the other Lark elements are needed.
    • Note that for operands and functions where we just call np, these are still explicitly defined. Could migrate these to the same architecture as the other builtins, but they are a lot simpler and are well defined so it may be overkill.
  • Properly handle the Python version issues in a separate PR
  • Frontend dev (and feedback for any required backend changes)
  • Validation on generated data (gemini data has limited value for "realistic" results...)
    • Do functions give sensible results, in particular smoothing
    • Do we handle pixel depth properly

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.11%. Comparing base (8572af0) to head (6d1956b).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   83.00%   87.11%   +4.10%     
==========================================
  Files          45       65      +20     
  Lines        2160     2724     +564     
  Branches      164      214      +50     
==========================================
+ Hits         1793     2373     +580     
+ Misses        335      320      -15     
+ Partials       32       31       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@patrick-austin patrick-austin marked this pull request as ready for review February 15, 2024 09:34
@patrick-austin
Copy link
Contributor Author

patrick-austin commented Feb 27, 2024

  • Added high level md summary of functions
  • Increased detail in Transformer docstrings
  • Add examples to postman

@louise-davies louise-davies changed the title Implement functions with Lark DSEGOG-289 DSEGOG-291 DSEGOG-292 DSEGOG-293 DSEGOG-290 Implement functions with Lark Mar 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 98.11321% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 95.54%. Comparing base (6ba3184) to head (035f38b).

Files Patch % Lines
...perationsgateway_api/src/records/export_handler.py 89.74% 1 Missing and 3 partials ⚠️
operationsgateway_api/src/records/record.py 97.36% 1 Missing and 2 partials ⚠️
operationsgateway_api/src/routes/functions.py 95.83% 1 Missing and 1 partial ⚠️
operationsgateway_api/src/routes/images.py 80.00% 0 Missing and 2 partials ⚠️
operationsgateway_api/src/routes/waveforms.py 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   94.99%   95.54%   +0.54%     
==========================================
  Files          50       70      +20     
  Lines        2837     3456     +619     
  Branches      297      365      +68     
==========================================
+ Hits         2695     3302     +607     
- Misses        105      108       +3     
- Partials       37       46       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@patrick-austin
Copy link
Contributor Author

Updated to work with export functionality from #104
Test failures will be fixed by #109 and #110

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