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

1089 full shore importer #1102

Draft
wants to merge 20 commits into
base: develop
Choose a base branch
from
Draft

1089 full shore importer #1102

wants to merge 20 commits into from

Conversation

mew-nsc
Copy link
Contributor

@mew-nsc mew-nsc commented Dec 7, 2021

🧰 Issue

Fixes #1089

🚀 Overview:

Created an importer for the CSV output of the full shore / DNA format

🤔 Reason:

To allow Full Shore / DNA data to be imported into Pepys

🔨Work carried out:

  • Collected unclassified sample data
  • Parse format 1
  • Parse format 2
  • Confirmed units
  • Clarified outstanding questions around the format
  • Tests pass

Confirmations

  • I have chosen reviewers for my PR.
  • I have chosen an appropriate label for the PR, adding interactive_review if reviewers will need to see UI
  • I have extended/updated the documentation in \docs folder
  • Any database content changes (Create, Edit, Delete) are recorded in the Log/Changes tables
  • Any database schema changes are implemented via alembic revision transitions
  • I have completed the mandatory sections of this document.
  • I have deleted any unused sections.

@mew-nsc mew-nsc self-assigned this Dec 7, 2021
@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 7, 2021 07:55 Inactive
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #1102 (928eb23) into develop (bb8d8b5) will increase coverage by 0.30%.
The diff coverage is 98.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1102      +/-   ##
===========================================
+ Coverage    80.03%   80.33%   +0.30%     
===========================================
  Files          110      111       +1     
  Lines        11889    12087     +198     
===========================================
+ Hits          9515     9710     +195     
- Misses        2374     2377       +3     
Impacted Files Coverage Δ
importers/full_shore_importer.py 98.71% <98.71%> (ø)
importers/wecdis_importer.py 97.68% <0.00%> (-0.94%) ⬇️
pepys_import/resolvers/command_line_resolver.py 94.60% <0.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60ad4cd...928eb23. Read the comment docs.

state = datafile.create_state(data_store, self.platform, sensor, timestamp, self.short_name)

location = Location(errors=self.errors, error_type=self.error_type)
lat_degs = float(lat_token.text) * (180 / math.pi)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be neater to use Python's math.degrees() function (as we do in the eag_importer). This will make it easier to track instances where we're converting from radians to degrees.

@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 10, 2021 08:18 Inactive
@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 10, 2021 08:19 Inactive
@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 10, 2021 11:15 Inactive
@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 14, 2021 16:25 Inactive
@IanMayo IanMayo temporarily deployed to pepys-timeli-1089-full--chlfik December 16, 2021 12:47 Inactive
@mew-nsc
Copy link
Contributor Author

mew-nsc commented Dec 16, 2021

This PR is left open as a draft PR pending answers about the units that the format uses. The code as written assumes some units:

  • Heading/bearing units are unclear. Implementation assumes radians
  • Speed units are unclear. Implementation assumes knots
  • Elevation units are unclear. Implementation assumes metres
  • Latitude/Longitude units are unclear. Implementation assumes decimal radians

These will need confirming before this gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importer for Full Shore format
2 participants