Adding get_nbdata

[briday1]

Allows users to request nbdata without needing to also get wbdata. This is particularly useful if you have a large CPHD file and you only want to look at the narrowband data.

[utwade]

What about this?

[wbvectors, nbdata]=ro.read_cphd('all', []);

[briday1]

Yes, I see that does get at the desired output.

I was thinking more from a discovery point of view, where there are functions that directly return the metadata, but not the narrowband data. From the read_cphd function and its documentation, it is not apparent that you can return the nbdata without the wbdata. In the example you provided, it still looks like it returns wbdata albeit it's empty.

My thinking was that there is a function read_cphd_data that gives you all 3 outputs wbvectors, nbdata, and meta.

If you go the route of instantiating a reader object, then you can get the metadata with the get_meta method. And if you wanted just the wbvectors output, you can use the read_cphd method with only one output defined. As a new user myself, it seems like it's natural to have a method to fetch the nbdata and (to me) it seemed like it was missing.

The content of this pull request seems (to me) like it fills in a gap of functionality that a newer user may be looking for. I do agree that your code example gives exactly the output that's desired, but it's not as apparent as a named method that gives just that content.

[utwade]

If we were to do that, it seems that should hold true for all phase history formats, not just CPHD. Perhaps there should be a

if exist(ro,'read_cphd')
readerobj.get_nbdata = ro.read_cphd('all', []);
elseif exist(ro,'read_raw')
readerobj.get_nbdata = ro.read_raw('all', []);
end

at the end of open_ph_reader.m?

The would provide the same functionality for all formats, even for readers for formats that we haven't built yet (assuming you call the generic open_ph_reader, and not one of the subclasses directly.)

[briday1]

So I went down the path that you suggested, but there were a few issues. Your method using the anonymous functions was complicated due to the fact that read_cphd_data and read_raw return two outputs. So the anonymous function is tricky in that case. Additionally, it doesn't let you specify the channel or the pulse_indices. I did like that it was a way to catch the other formats (cphd30 and gotcha), but it still only caught it if you initiated it with the open_ph_reader and not their specific methods.

What I have done in my most recent commits was to implement get_nbdata for all of the phase history formats in the repo. This isn't as clean as the idea you listed above, but this only has to be done once for each standard, which shouldn't happen too terribly often I imagine.

Then, the way I implemented it was to use your idea of calling the read_data method and only getting the nbdata portion by setting sample_indices to an empty array.

Lastly, in editing the open_gotchapr_reader.m file, it looked like there was a channels input parameter to read_data that was unused, so I deleted it. It seems that for gotcha the num_channels is always 1 as specified in the metadata constructor.

Let me know what you think!

[utwade]

To be clear, I'm not opposed to the new method, but I do want to work through a few issues before I accept the PR. Appreciate your patience with my pickiness.

I'm not sure that I understand why read_cphd_data and read_raw would be a problem. They don't call the new get_nbdata method, so it shouldn't be an issue for them, and the new method only returns 1 output, so anonymous function should be fine, right?

As for the gotcha reader, I think we have to maintain the unused input parameter because it's conceptually a subclass of a larger phase history reader class, so it has to maintain the same API across in that family. Someone may write code generically for CPHD, and this allows that same code to run on gotcha data, just as if it were a CPHD, even if it only has a single channel. If the CPHD code sees that the data has only one channel and asked to run on that one channel, we don't want that to cause it to crash.