SpatioTemporal / STAREPandas

STAREpandas adds SpatioTemporal Adaptive Resolution Encoding (STARE) support to pandas DataFrames. https://starepandas.readthedocs.io/en/latest/
MIT License
4 stars 1 forks source link

why do we import starepandas in some modules #140

Open NiklasPhabian opened 1 year ago

NiklasPhabian commented 1 year ago

e.g. STAREPandas/starepandas/io/granules/__init__.py imports starepandas. Why?

mbauer288 commented 1 year ago

Okay, I found that commenting out the explicit imports of pystare and starepandas in all the modules under starepandas/io seemily has no effect because if you look at the global namespace like sys.modules.keys() or a local namespace granule.__dict__.keys() both starepandas and pystare are already available due too much earlier definitions.

Indeed, starepandas/io/__init__.py already knows about pystare and starepandas before any of the enclosed modules are imported.

Anyway, I suspect is this just cosmetic; the python standard of ignoring previously imported modules makes these imports redundant (python is pretty good at avoiding circular referencing and imports).

That being said, these imports immediately caught my eye as being "odd" and made me (and you) spend time thinking about it, so perhaps we should carefully check if in fact they are unnecessarily, and if so, remove them for clarity.

Of course imports like in starepandas/io/granules/ssmis.py:

    from starepandas.io.granules.granule import Granule
    import starepandas.io.s3

are perfectly in order as they only pull Granule into the local namespace of ssmis.py. I believe from .granule import Granule does the same thing with a bit less verbiage.

Using explicit dot-notation to call s3 functions in ssmis.py (ds = starepandas.io.s3.nc4_dataset_wrapper(scp)) is clearer IHMO than if we used from starepandas.io.s3 import nc4_dataset_wrapper and ds = nc4_dataset_wrapper(scp). Both are valid of course.