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

WorkhorseParser - wrong assignment of velocity components #741

Closed
ocehugo opened this issue May 27, 2021 · 1 comment
Closed

WorkhorseParser - wrong assignment of velocity components #741

ocehugo opened this issue May 27, 2021 · 1 comment
Labels
Type:bug Something is missing or wrong Type:Reprocessing Reprocessing of files required Unit:Instrument Reader Instrument parsers and related Unit:TimeSeries timeSeries related

Comments

@ocehugo
Copy link
Contributor

ocehugo commented May 27, 2021

This bug affects v2.6.11 & v2.6.12

When reading ENU datasets with the newly refactored workhorse Parser, the variable mappings are reversed and a wrong assignment is being done. The current bug lies in assigning velocity1->VCUR and velocity2->UCUR, while the correct is the reverse.

This only occurs for ENU datasets.

The problem is located in the import_mappings on the recently refactored Workhorse parser (v2.6.11+). It went undetected because even the tests got the typo (see +Workhorse/import_mappings.m). We are also missing a content regression test against ENU files, which would have picked the problem.

The origin is likely related to a wrong copy/paste/edit since the original workhorseParser firstly defined a VCUR variable, then a UCUR variable, but with the correct assignments.

Thanks to Tim Austin for reporting

@ocehugo ocehugo added Type:bug Something is missing or wrong Unit:Instrument Reader Instrument parsers and related Unit:TimeSeries timeSeries related Type:Reprocessing Reprocessing of files required labels May 27, 2021
ocehugo added a commit that referenced this issue Jun 10, 2021
This fixes bug #741, where both the code
and docstring tests were assigning U (V)
velocity to beam number 2 (1) for
Teledyne workhorses ENU ADCP datasets.

Apart from the code/test fix, a new `xunit`
regression test is now provided,
where we compare the results of the reading with
a previous file that was read with
version 2.6.9 (before the
changes were introduced).

Finally, the commits also include
a small refactoring in `import_mappings`
so variable assignment is more explicit,
and in `workhorseParse` with better
error msg at the import step.
@ocehugo
Copy link
Contributor Author

ocehugo commented Jun 10, 2021

closed by 3806980

@ocehugo ocehugo closed this as completed Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:bug Something is missing or wrong Type:Reprocessing Reprocessing of files required Unit:Instrument Reader Instrument parsers and related Unit:TimeSeries timeSeries related
Projects
None yet
Development

No branches or pull requests

1 participant