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

Proposed reorganization of AD2CP data format #719

Draft
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Jun 5, 2022

In v0.6.0 we left out AD2CP data because it takes quite a different flavor from all the other sonar models. Now we are in the position to reorganize the data format based on the previous experience.

In this PR I put in a markdown file that details my proposed changes of the data format. The file is put under docs/source but my intension is not to merge this file but use this PR as a place where we can do line by line exchanges/discussions if needed.

@emiliom @imranmaj : I think it may work better if @emiliom and I meet first to settle down the proposed changes (which will be become new commits to this PR), and then @imranmaj and I meet to go through the required changes.

@emiliom: I can give you an overview of how this instrument operations and the main differences from the other sonar models. The variables are in such a way that can be grouped into a few conceptual groups, so even though there are a large number of them I think it won't be too hard for us to make decisions.

@imranmaj: There are a few questions that are independent of the format changes. We can discuss those in parallel.

@leewujung leewujung added enhancement This makes echopype better data conversion labels Jun 5, 2022
@leewujung leewujung added this to the 0.6.1 milestone Jun 5, 2022
@leewujung
Copy link
Member Author

leewujung commented Jun 20, 2022

A new question as I test the new parser/organization from #731 using data from my fieldwork last year:
I think it'll be useful if we can save the entire configuration string as an attribute (or another appropriate form) in the converted data. This will ensure no loss of information, since right now the config info is parsed but not save.

This is an example of the config string saved as a txt file:
https://drive.google.com/file/d/1q-Njr7ZlebLiHjnZElsRlcYMksudPw9q/view?usp=sharing

@imranmaj has a nice function in the parser to convert the config string to dict, and it is used in the parser for getting some specific info, but the config params are not saved in general.

@emiliom : Do you have recommendation on how this should be saved? I see this as similar to the config XML that is saved in its entity for EK80 in the Vendor_specific group:

ds.attrs["config_xml"] = self.parser_obj.config_datagram["xml"]

@emiliom
Copy link
Collaborator

emiliom commented Jun 20, 2022

Given that config_xml attribute precedent, it seems to me that what you're proposing makes sense. Plus, preserving config info is always a good thing, especially if there are no downsides. You could store the raw text file as is, or convert it to a dict then save it as a JSON string. The only comment I'd make about the format of that dict is that each of its items (eg, item key "BEAMCFGLIST") often has content in the form of a list of dicts, eg:

"BEAMCFGLIST": [dict1, dict2, ...]

@leewujung leewujung modified the milestones: 0.6.1, 0.6.2 Jun 30, 2022
@leewujung leewujung modified the milestones: 0.6.2, 0.6.3 Jul 28, 2022
@lsetiawan lsetiawan marked this pull request as draft August 10, 2022 21:43
@leewujung leewujung removed this from the 0.6.3 milestone Oct 13, 2022
@leewujung leewujung changed the base branch from old_dev to dev December 23, 2022 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data conversion enhancement This makes echopype better
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants