check for zero size landgrid before calling .min

[poplarShift]

Hei, I seemed to need this fix a while ago. Unsure whether there is some root issue why I got zero size landgrids in the first place, but since opendrift hardly ever raises errors anyway, we can probably get away by emitting a warning here too.

[knutfrode]

Hi, I have never encountered an error with landgrid of size 0, and can also not see how that could happen? But it is then perhaps better to raise an error so that the cause of this can be identified and fixed, instead of masking it (albeit with a warning). What do you think @gauteh ?

[gauteh]

Hi, Yes, I agree. Maybe better to put an assert there, if it becomes a problem in the future and should it should not stop the simulation it can be changed then. It probably indicates some issue with the input landmask model that needs to be fixed for meaningful simulation.

[knutfrode]

Yes, so you could simply do assert landgrid.size == 0

Generally, it is best to raise errors when something is wrong. However, the main loop of OpenDrift is made robust with try-except to ensure that operational simulations (e.g. search&rescue and oilspills) will complete (but issue warning) even if there is a problem with some of the readers (e.g. a corrupted ocean current file or hangig OPeNDAP server), as then normally a secondary ocean model will fulfill the task of providing forcing data. But this is also depends on the default configuration of each model. E.g. for Leeway and OpenOil, the simulation will stop (but not crash) if no winds or currents can be retrieved (fallback values are NaN) for a given time step. For the OceanDrift module, winds and currents have fallback values of 0, so it is possible to run a simulation without winds and/or currents, e.g. for conceptual studies.

Btw, if you provide stop_on_error=True to run( ) the simulation will stop (crash) on the first error in main loop, but then also output netCDF file will probably not be readable.

[gauteh]

I have started work on making this more granular. The first step is to make more specific errors, which you can see here: https://github.com/OpenDrift/opendrift/blob/master/opendrift/errors.py , maybe one of those can be used. The idea is that if e.g. a reader gives an OutOfTimeDomain error or similar it can be thrown out if the simulation has passed its use (considering the additional nuances of that).

[poplarShift]

Yes, I was also very perplexed, but unfortunately I did not keep the log files I think. But it was a long simulation (several days of CPU time) so making an MWE would have been a nightmare anyway.

Also, I am very positive to raising errors that can be handled appropriately! I will update the PR with assert next time I'm in the code.

[poplarShift]

I assume that whether or not landgrid.size can be 0 is what comes back from this routine: https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/operators/readerops.py#L46 (since all of the other covers_positions return booleans, not arrays or anything else one could index on.) Is there any reason this cannot be zero size? What do we know about what is combined?

[gauteh]

I think the landmask reader uses this function: https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/basereader/variables.py#L222

[poplarShift]

Ah, ok. Then why can https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/basereader/variables.py#L222 not return a zero size array? If e.g. all particles have drifted out of the area of the land reader?

[knutfrode]

Hi, should I close this PR, and replace your changes with assert landgrid.size == 0 ?