search

pytroll / satpy

Python package for earth-observing satellite data processing
http://satpy.readthedocs.org/en/latest/
GNU General Public License v3.0
1.06k stars 292 forks source link

Make `_get_sensor_angles` and `_get_sun_angles` public #2148

Open simonrp84 opened 2 years ago

simonrp84 commented 2 years ago

At present, the two functions listed above, which are in modifiers.angles are private (hence the leading _). I frequently use these functions in my own processing chains and I suspect others do too - solar and satellite angles are a commonly used thing.

Can we remove the underscores and make these two functions public? Of course, they're useable as-is but linters tend to complain about accessing private functions. Happy to make a PR for this, but wanted to start with an issue in case there's some reason I've missed that these functions are the way that they are.

djhoese commented 2 years ago

The main reason they are private is so people don't use them directly and the interfaces could change in the future if needed. The get_angles function was meant to be the only public access for now. I'm not too against making these public but the other big part of these decisions was that this "angle stuff" was supposed to get a more Scene and compositor friendly interface in the future. I still don't know what that functionality would look like so maybe more public is the way to go for now.

On mobile right now, but we need to be careful that the interfaces are similar and caching gets used as expected.

simonrp84 commented 2 years ago

Coming back to this, the problem with get_angles is that it returns both sun and sensor angles, even if the user only needs one or the other of these.

How does dask handle that? If the user calls get_angles but only actually uses the sun azimuth + zenith then is dask smart enough not to bother computing the satellite azimuth + zenith? If so, there's no need for the _get_sun_angles and _get_sensor_angles to be public...but I'm unsure how to test this.

djhoese commented 2 years ago

There are dask diagnostic type things or you can visualize the dask array by doing .visualize() on it which can generate an SVG of the overall dask graph. If you generate the graph for one of the sun angles and the graph contains things related to the sensor angles then that's not good. From what I can tell the sun angles should be separate from each other. The satellite angles are generated together. So if you compute either of the sun angles (azimuth or zenith) then I think it should be fine. If you generate either of the satellite angles then it will automatically compute the other satellite angle as a side effect.

djhoese commented 11 months ago

As discussed on slack today, I don't have a problem with these being public, but as I said in my first comment near the top I had hoped that we'd come up with some better interface that was more Scene or reader-centric. That's going to take too long and requires too many discussions.

I do want to note that you (@simonrp84) noted a ~1.15s slow down when using the private solar-only function versus the get_angles function with caching turned off. You and I confirmed this was not due to the satellite angles being generated (that should contribute a lot more), but it seems something about the dask graph creation was taking a long time.