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

Change output of open_raw to an EchoData object #282

Merged

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Apr 19, 2021

This PR:

Note:

  • open_raw only works with parsing a single raw data file.
  • open_raw and open_converted are now under echodata/open_file.py. I was debating if open_raw should remain in the convert module but eventually moved it to under echodata. Let me know if you think otherwise.

Todo for later PRs:

  • still need to implement open_mfraw to parse multiple raw files and combine the parsed data into attributes of a single EchoData object.
  • we should remove the Convert class since its function as an interface for using ParseXX and SetGroupsXX objects can now be substituted with Echodata

Updates

  • Convert is now deprecated with all functionalities provided directly by Parse and SetGroup objects
  • Next steps are to:
    • take care of specific functions under echopype/convert/api.py, such as _validate_path and to_file, to just provide with 1-to-1 (input/output) mapping
    • Other singular changes in Update open_raw leewujung/echopype#3

@leewujung leewujung requested review from emiliom and lsetiawan April 19, 2021 05:32
@leewujung
Copy link
Member Author

@lsetiawan : Could you take a look at the use of storage_options and output_storage_options in EchoData this PR? I am pretty sure when trying to transplant _validate_path and associated functions I broke your test_convert, because the output of open_raw is now an EchoData object instead of a Convert object. Thanks!

@lsetiawan
Copy link
Member

Looks like you integrated PR #281 into here is that correct?

elif convert_type == "netcdf4":
return out_f

def _to_file(
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... looks like you integrated a lot of the stuff from Convert Object ... Not sure if this is the best way to go because I can see EchoData ending up being convoluted ... I think it might be better if you keep all of these functions in the Convert Object and use that within EchoData.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, those transplanted functions are my biggest hesitation, as in whether to keep them in Convert or move them to EchoData. (I was thinking about getting rid of the Convert object!) But looking at this again maybe it'll be better that way.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I think it's okay to keep the Convert object. Since we know it already works and this would split up the functionalities, and we can just test individually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh now I remember why I did that: the main thing is that now the set groups functions return xr.Dataset and are linked directly to attributes of EchoData in open_raw. That's why I felt the Convert class can actually be removed and substituted by the same functionalities in EchoData. This does make EchoData more convoluted though I agree. Hmm... thoughts?

@@ -14,7 +14,7 @@
import xarray as xr
import pytest
from pathlib import Path
from ..convert import open_raw
from ..echodata import open_raw
Copy link
Member

Choose a reason for hiding this comment

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

For tests, it's better if you don't do relative paths since echodata should be installed before you run pytest. So you can just do from echodata import ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, cool, thanks for explaining this!

@leewujung
Copy link
Member Author

Looks like you integrated PR #281 into here is that correct?

Yes, wanted to make sure I got your changes in before starting this.

@leewujung
Copy link
Member Author

Noting conversations in leewujung#3 for more changes on this PR.

@leewujung
Copy link
Member Author

@lsetiawan thanks for the changes! I just put updates the PR message to include next steps and will merge this now.

@leewujung leewujung merged commit 57c50dc into OSOceanAcoustics:class-redesign Apr 20, 2021
@leewujung leewujung deleted the open-raw-to-echodata branch April 29, 2021 03:31
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.

2 participants