BatBot initial testing and integration #305
Conversation
BryonLewis
changed the title
…into batbot-integration
naglepuff
left a comment
naglepuff
left a comment
There was a problem hiding this comment.
Overall looks pretty good, excited to get this integration done. I had some feedback, but nothing too major.
| width = models.FloatField() # pixels | ||
| height = models.FloatField() # pixels |
There was a problem hiding this comment.
For these two fields specifically, which I believe are just a pixel count, would it make sense to just keep them as integers?
| width = models.FloatField() # pixels | ||
| height = models.FloatField() # pixels |
There was a problem hiding this comment.
Same question here. Would it be more accurate to use integers for height/width?
| "sentry-sdk[celery,django,pure_eval]", | ||
| "gunicorn", | ||
| "geopandas>=1.1.1", | ||
| "batbot", |
There was a problem hiding this comment.
Is there anything that we can remove from our direct dependencies in favor of batbot? Maybe some (or all) of those listed under the # Spectrogram Generation comment?
| Noise Filter {{ transparencyThreshold }}% | ||
| </v-card-title> | ||
| <v-card-text> | ||
| <p>Removes amplitudes below the percentage</p> |
There was a problem hiding this comment.
| <p>Removes amplitudes below the percentage</p> | |
| <p>Removes low-intensity background noise in the spectrogram</p> |
Feel free to disregard this suggestion, but the wording struck me as a little bit technical, which might be fine, but I'm not exactly sure how much meaning it will have for the target user.
| left: -geoViewer.value.bounds().right * 0.1, | ||
| top: 0, | ||
| right: originalDimensions.width, | ||
| bottom: originalDimensions.height, | ||
| right: geoViewer.value.bounds().right * 1.1, | ||
| bottom: geoViewer.value.bounds().bottom, | ||
| }; |
There was a problem hiding this comment.
What is the significance of 0.1 and 1.1 here?
There was a problem hiding this comment.
it's left/right padding around the spectrogram. We could change it to something more static instead.
| # TODO: Disabled until prediction is in batbot | ||
| if config and config.run_inference_on_upload and False: |
There was a problem hiding this comment.
Is there an issue in batbot or BatAI tracking this? It would be nice to have a link to an issue in either of the GH repos here while we wait for this feature.
It might also be nice to add some sort of banner in the UI somewhere that tells users that prediction is disabled for now.
| # TODO: Disabled until prediction is in batbot | ||
| if config and config.run_inference_on_upload and False: |
There was a problem hiding this comment.
Same idea here about a linked issue/visibility for users on the front-end web side.
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| results = generate_spectrogram_assets(recording.audio_file, tmpdir) | ||
| # Copy the audio file from FileField to a temporary file |
There was a problem hiding this comment.
Is this needed because of how batbot handles spectrogram generation?
There was a problem hiding this comment.
yeah it no longer is using a fileHandle or something like that it. It needs a real file.
There was a problem hiding this comment.
It looks like this file has a lot of duplicated code in it. Can bats_ai be added as a dependency so we can import the pydantic models and spectrogram generation code?
2 participants
resolves #304
./scripts/batbot/./scripts/contours./batai/core/utils/batbot_metadata.pyfile to manage batbot and the resulting datagenerate_spectrogram_assetsmeant to be a drop-in replacement for the older generate_spectrogram_assets but this one may contain additional information in the future (contours + curves and knee, heel, toe points)