save vs set in class redesign

[emiliom]

(I've enabled the new repo Discussions feature ... First time using it, so let's see how well it works!)

I'm still familiarizing myself with the way things are structured in the class redesign. One observation I'd make is that the distinction between setting and saving is confusing. For example, the SetGroupsEK60.save method calls several set_ methods, each of which sets up and populates a specific nc4 group ... then calls the utils.io.save_file method to do the saving (writing). So, "set" does more than just setting: it also writes. I find that variability in meanings confusing and hard to keep track of.

Not to diverge too much from the main topic here, but something similar occurs with SetGroupsBase.convert_obj. In reality, convert_obj refers to something more specific than the broader "convert" functionality. It's a "parse" object. That's clear in the comment (# a convert object ParseEK60/ParseAZFP/etc...), but when encountering convert_obj elsewhere, and you're new-ish to the code, it's confusing. In this case, I think I'd rename convert_obj to parse_obj (or parser, or parse).

[leewujung]

(This is interesting -- is Discussion supposed to be for discussions and Issues for real issues with the code?)

About SetGroups...: I forgot how the name came about, there was probably at some point of time when setgroups only did set groups, but not anymore, hah. Perhaps SaveGroups... is a better name, and SaveGroupsEK60.save should be SaveGroupsEK60.save_all_groups?

The convert_obj thing makes sense: I can make that global change in these coming few days as I go through all the refactored classes.

[emiliom]

(The Discussions feature tries to address the long standing challenge of wide ranging, roaming issues that in reality are broader discussions rather than specific issues addressable with narrow PR's)

From my funky, GPS data perspective of wanting to short circuit the convert workflow to not have to write a file, I'd like to have the save steps be separate from the set steps. In other words, SetGroups ... would return the xarray dataset, and the user would have to perform a separate "save" step to do the actual saving of each group, or all groups in one step. But this may be too big a change at this time, best left for later.

[leewujung]

I think it should be pretty straightforward to make that separation: making the set_beam type of methods to return the xarray datasets, and have a separate save method to actually do the saving of different groups. Then users with knowledge go under the hood can grab what they need.

The type of short-circuiting you wanted though I think requires actual changes in the parser, and I think it'll be great to make it an issue to be resolved in one of our milestones.

[leewujung]

(Forgot to add: I like the distinction between Discussion and Issue! Somehow it feels that the discussions can be much more free-form here and some part of the conversation could naturally evolve into a focused issue to be addressed by a PR. )

[emiliom]

Exactly! And there are some custom features that are more forum-like, such as replying to a specific comment, like I'm doing here. This is nicer for broad ranging discussions.

[emiliom]

I think it should be pretty straightforward to make that separation: making the set_beam type of methods to return the xarray datasets, and have a separate save method to actually do the saving of different groups. Then users with knowledge go under the hood can grab what they need.

Good to hear! I think I'm hearing that this change could be made w/o impacting the vast majority of users, who would still just call to_netcdf and such.

The type of short-circuiting you wanted though I think requires actual changes in the parser, and I think it'll be great to make it an issue to be resolved in one of our milestones.

It goes far in that direction, though. Currently the short-circuiting I'm doing involves calling the parse_raw method (after instantiating a Convert object), then grabbing and parsing the GPS datagram. I have to extract code from echopype to do these things externally. Separating set-group from save functionality would allow me to call echopype code directly w/o having to extract the code. I think what you're referring to is the additional capability to only extract the GPS datagram w/o doing anything at all (ie, no parsing) with the acoustic and other datagrams. If so, that'd be great, but it's an additional capability on top of what I'm currently doing.

[leewujung]

Good to hear! I think I'm hearing that this change could be made w/o impacting the vast majority of users, who would still just call to_netcdf and such.

Yes! Could you raise an issue for this? That'll give me some motivation and help us keep track of this. :)

I can write an issue about changing the parser as a backburner, since it involves a bit more detailed info about the echosounder type.

[emiliom]

Could you raise an issue for this?
Done. #228