(corner case) Ban negative indices about mocpy HOT 6 CLOSED

It was discovered due to the -1 that is used as padding in cdshealpix.neighbors when a cell has three neighbors instead of four. This generates a set of healpix's cells with spurious -1 among them.

I like FX's idea to define a decorator on the python's side. This way there is only one decorator to maintain and we can apply it to all methods that accept healpix cells (just as we already do with the validate_lonlat decorator that ensures that all coordinates sent to the rust side are in degrees, in icrs, and are wrapped)

from mocpy.

How does the conversion from python to rust type work?
Could this issue be solved on the Rust side?

from mocpy.

It silently accepts negative integers from python and converts them into unsigned integers in rust https://pyo3.rs/main/conversions/tables (which lead to crazy values)
I think it fits in this pyO3 issue but I'm not sure PyO3/pyo3#3226

Blanketing our integers before sending them to the rust side shouldn't affect the performance too much if it's your concern?

from mocpy.

My concern was rather about having a single checkpoint. I thought this could be managed on the Rust side, at conversion time, but it seems I'm wrong.

from mocpy.

maybe @fxpineau has an idea on how to do this better?

from mocpy.

In which cases do we end up with negative HEALPix indices that are passed from Python to Rust?

Given that pyo3 do not produce and error (so far) when converting from a negative integer to an unsigned integer, we may consider at least two opinions:

• prefer to have a single checkpoint on the Rust side: technically, I could accept signed integers on the Rust side, and then filter and cast into unsigned integer. Pros: no duplicated code / no risk to forget to check. Cons: 1. performance penalty for cases in which we are sure that all indices are ok (i.e. when checking is useless), 2. write code that would not exist if pyo3 behavior was to return an error;
• prefer having a method checking (and cleaning) an array of HEALPix index on the Python side, and call it when necessary: we consider that the Rust method (accepting only unsigned integer) is a contract that must be fulfill, and the Python code calling the method has the responsibility to ensure it, identifying beforehand cases in which the contract may be broken (pros/cons are reversed with respect to the first bullet pros/cons).

I personally have a marked preference for the second bullet (check on Python side), possibly isolating and decorating with a check the call to the Rust method to mitigate the 'cons' (no duplication/no risk to forget, i.e. single checkpoint on the Python side).

from mocpy.