Fetch skips downloading data for some instruments

[j-atkins]

It appears that virtualship fetch will only download data from Copernicus Marine Service by virtue of the fact that “ship_underwater_st_config” and “adcp_config” are specified in ShipConfig objects as standard. Meanwhile, the presence of other instruments e.g. "DRIFTER" and "ARGO_FLOAT" in schedule.yaml does not actually prompt a data download. I notice this is related to #122 which has been closed, but I think there could still be problems in the current implementation?

To elaborate, I think there are two problems here:

1) _fetch() will always download Bathymetry, T, S and U & V by always satisfying the following if statement in https://github.com/OceanParcels/virtualship/blob/main/src/virtualship/cli/_fetch.py:

    if (
        (
            {"XBT", "CTD", "SHIP_UNDERWATER_ST"}
            & set(instrument.name for instrument in instruments_in_schedule)
        )
        or hasattr(ship_config, "ship_underwater_st_config")
        or hasattr(ship_config, "adcp_config")
    ):

This if statement always passes as True because hasattr(ship_config, "ship_underwater_st_config") and hasattr(ship_config, "adcp_config") are always True, I think because they are always present as attributes in the ShipConfig object (even if set to None).

2) Theif statement lines to download data for "DRIFTER" and "ARGO_FLOAT" added in #122 (e.g.if "DRIFTER" in instruments_in_schedule:) appear to always result in False. The string doesn't seem to ever be found in instruments_in_schedule. It's not immediately clear to me why this is the case because it looks like it should on first glance, but I will look into it.

In summary...

1) is currently more of an 'inefficiency' as it's not necessarily stopping anything else from working but rather always downloading data when it's not necessarily always needed. Perhaps hasattr() could be replaced by something which means the statement is False if "ship_underwater_st_config" or "adcp_config" are None in ShipConfig objects?

2) is a bug which is preventing the download of extra data such as biogeochemistry so I will look into fixing this before I can carry on with incorporating biogeochemistry workflows as per #171. I suspect this hasn't been an issue up to now because VirtualShip has so far only been used for physical variables which are always downloaded due to what's been outlined in 1) and the data is therefore available for DRIFTER and ARGO_FLOAT instruments anyway by the time it comes to running virtualship run.

[j-atkins]

I believe there are a couple of quick fixes which can be implemented:

For 1), is not None statements can be used instead of hasattr()

For 2), the relevant instrument can be identified when searching for its string e.g. "DRIFTER" in str(instruments_in_schedule)

I will make the fixes and make a PR.

[ammedd]

Great that you came up with some solutions. I think you understand the gist of what we were trying to implement. To reduce extensive data downloads, but apparently with some buggy code. Just make sure you test your implementation :-)