JOSS review: functionality documentation & testing about virgo HOT 20 CLOSED

@astrom-tom Is something like this sufficient?

from virgo.

That being said, I'll look into building a GNU Radio-based I/Q to .dat parser. This would allow you to take the raw data from any instrument and view, process and analyze the data with Virgo however you wish (as long as the essential observation parameters/headers are known of course).

Yeah, that sounds cool. I think this would be a great addition. With my hat of reviewer on, I want to add that in principle I don't consider it as being required to be part of the reviewed version, but more as a general idea for extension post-review.

from virgo.

In-code documentation has been implemented for every function.

For example,

>>> import virgo
>>> help(virgo.predict)

now clarifies the role of the function and the properties of each parameter:

predict(lat, lon, height=0, source='', date='', plot_sun=True, plot_file='')
    Plots source Alt/Az given the observer's Earth coordinates.
    
    Args:
            lat: float. Observer latitude [deg]
            lon: float. Obesrver longitude [deg]
            height: float. Observer elevation [m]
            source: string. Source name
            date: string. Date in YYYY-MM-DD format. If no date is given, it defaults to today's system date.
            plot_sun: bool. Also plot Sun position for reference
            plot_file: string. Output plot filename

Since it is unlikely that a reviewer would have access to appropriate hardware, we have included an example observation file obtained with Virgo on the repository for software testing and verification purposes, as described here. Please let me know if this is sufficient, or if you were envisioning something else.

from virgo.

Let me reply to this one too :):

docstrings: Great addition!

I think having unit tests (which would be testing every function for every type of input/output, but of course there are very pragmatic ways to go about this) can help you catch subtle issues (e.g. input generating wrong output, or error not being caught, etc) better than having a single test for the whole, but in general I would say that giving users who just installed the tool a way to prove that the install on their system runs as expected is a good start (and I think that is pretty much what you did). I think that should be enough for the review.

Does the manual test you define cover the whole functionality in the code? I will run it and if I have feedback on them, you'll hear it :).

from virgo.

@ygrange - I just extended the script so that it is more complete in terms of tests/examples (a384417).

Unless a dependency has not been properly installed or some other error is raised, the expected output (along with four output image files: observation.png, map.png, profile.png, cas_a.png) is this:

HPBW: 9.85
Galactic longitude: 82.3736020515371, Galactic latitude: -3.9909816848586024
RA: 5.156896049338616, Dec.: 68.99050645138365
A_e: 35.46948257424232
Gain: 0.07964064805982826
NF: 0.4948536307991825
T_noise: 169.61902581372294
Frequency: 1427583133.3333335
Wavelength: 0.21112144929577464
SEFD: 671.9156130666667
SNR: 127.79038214214714
G/T: 20.70581074285707

from virgo.

Yeah, that's a nice way to show the install was successful (you may want to consider change it from joss_example to something like sanity_check (or something more appropriate) but I totally leave that to you. You may want to mention near the installation instructions that you can use this example to test the installation (and at the same time it is a nice example of functionality; I've just downloaded some Dwingeloo telescope data from CAMRAS' public page to test with and I am using this one as entry point :) ; I can tell you I do get a (partial) plot).

from virgo.

The thing between parentheses brings me to the next question: is it sensible to want to use Virgo to download some public data and process it with the plot function? Because in that case it would be nice to have a small section in the docs that tells you what properties the data should have. I'm sure the data I am using to play with is not very appropriate (I noticed when trying to manually debug a bit that it seems to contain nans that do not get caught, but anyhow I get a bunch of numpy warnings on inappropriate numbers further on and I do not know whether that would be because of the telescope settings, or that I somehow should set certain features on and off.
(I hope you get the point I am trying to make here :) ).

from virgo.

Yeah, I was just looking through the public CAMRAS data archive because I was curious how you were planning to go about this. Ideally, you want to use the .plot function to visualize data generated with Virgo's .observe function, because the format is pretty specific. I know that the Dwingeloo telescope is equipped with a USRP SDR device (which is perfectly compatible and has been extensively tested with Virgo), but the data found on the archive is for the most part post-processed in all sorts of unique ways, so it would be a bit tricky (though not impossible) to parse some raw data and view them with Virgo, although I don't know how useful that would be.

For example, I could run the exact same code on an 18-meter telescope equipped with the same USRP model and it would be a test of similar quality I think, since the interface is basically identical (only the antenna aperture and RF frontend is different of course, but that isn't really relevant to such tests). If you'd like to avoid the hassle of observing with Dwingeloo etc., I could quickly acquire and share an example 18m.dat any time for you to test .plot against if that helps. You are of course welcome to provide any custom parameters for the .observe function by the way!

from virgo.

I guessed that CAMRAS had somehow been tested by looking at the author list :). I tried it with some satellite observations (basically because those seemed raw data to me) from here https://charon.camras.nl/public/satellites/ (selection mostly based on which link does not give me a 404 error. That gives me one plot that actually contains data (and a bunch of errors which probably have to do with NaNs in the data).

Yeah, I'd love to have a .dat set, but I assume it won't be too different from the example already in the repo? Would having some sort of boundary conditions to data on which the plot function would work (I realise this is much simpler said than done) so that as a user I could judge whether I can use some public data or not ("observed with Virgo" is of course a nice zeroeth order boundary condition).

from virgo.

Just to clarify, during observing, Virgo doesn't spit out raw I/Q data (like the ones found on the CAMRAS archive you linked), and here's why: raw I/Q data files contain all acquired information about the recorded signal (pre-FFT/discretized time-domain data points). The problem is that this information can get pretty large at high sample rates, long integration durations and ADC sample depths (bits), which can obviously get very impractical in the real world. The important thing to note, however, is that for many/most observations, raw I/Q information is not at all necessary.

Virgo tackles this problem by running the FFT (WOLA or plain computationally-inexpensive Fourier transform filterbank), channelizing and partially integrating the acquired data on the fly (in real time). This drastically reduces the data size from sometimes TBs to only a few MBs or GBs of information, containing nothing more than the user needs.

Therefore, the output .dat binary file produced by the package only contains FFT samples, and when processed with Numpy, this gets expanded to a 2D array (where each column represents the FFT bin/channel and each row represents the corresponding partially-averaged time bin). virgo.plot then takes care of processing and analyzing the data to produce each plot and apply all sorts of computations to the data given the user's input.

That being said, I'll look into building a GNU Radio-based I/Q to .dat parser. This would allow you to take the raw data from any instrument and view, process and analyze the data with Virgo however you wish (as long as the essential observation parameters/headers are known of course).

from virgo.

Hey!
So I went through the documentation about the functions here. I am happy to see that the functions are documented. Nevertheless, you always document the parameters of the function but not the output. For example. the beamwidth function surely has an output. This should be properly documented.

I will comment on the testing in another comment

from virgo.

Just added the output type for all functions. Hope it's a bit more obvious now.

from virgo.

Just added the output type for all functions. Hope it's a bit more obvious now.

is it in the website documentation as well?

from virgo.

Yeah, it's here. I suppose your browser cache will update shortly, but this is what it looks like:

from virgo.

Oh ok. I think it would be clearer to have, below the 'Parameters' list, the list of output (in the same format at input). Also, as you precise the unit of some of the parameters (that's awesome), you should precise the unit of the output to be complete.

from virgo.

Could you please clarify what you mean by 'list of output'? What would this look like for something like the beamwidth function for example which only returns one value?

As for the output units, that's a good point. Will add them.

from virgo.

Let's take this example: You have the list of parameters format like

Parameters: -S
-sefd
-t
-bw
...

It would be nice to have also the list of ouputs in the exact same format:

Returns: - SNR, signal to noise ratio, unitless, float

from virgo.

an example, in the code for a numpydoc doctstring:

def function_with_types_in_docstring(param1, param2):
    """Example function with types documented in the docstring.

    `PEP 484`_ type annotations are supported. If attribute, parameter, and
    return types are annotated according to `PEP 484`_, they do not need to be
    included in the docstring:

    Parameters
    ----------
    param1 : int
        The first parameter.
    param2 : str
        The second parameter.

    Returns
    -------
    bool
        True if successful, False otherwise.

    .. _PEP 484:
        https://www.python.org/dev/peps/pep-0484/

    """

from virgo.

Perfect!

from virgo.

For the testing part, I understood that the test you proposed is to make sure that the installation was done correctly without error. This is great but I am not sure this is what JOSS is referring to when they talk about testing. I will check with the editor and come back to you.

--> OK, the checklist for the review says: 'Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?' so I guess your example would serve as the manual step they are referring to. I would say that now we are all good. I will close the ticket.

from virgo.