search

desihub / redrock

Redshift fitting for spectroperfectionism
BSD 3-Clause "New" or "Revised" License
21 stars 13 forks source link

Dependency on desispec introduced since version 0.19.0 #305

Closed amolaeinezhad closed 2 months ago

amolaeinezhad commented 2 months ago

Since version 0.19.0, the redrock.templates module includes a call to from desispec.coaddition import coadd_cameras. This addition creates a dependency on the desispec package. However, it's worth noting that the coadd_cameras feature introduced by this call has not been utilized.

Maintaining Redrock's independence from other DESI dependencies has traditionally been a significant advantage. Introducing unnecessary dependencies could complicate the installation process and increase the package's overall footprint.

moustakas commented 2 months ago

Looks like this was added by @abhi0395 as part of #283 but for no particular reason-- https://github.com/desihub/redrock/pull/283/files#diff-cb77484e5becf510c166b87f21fcfc65025d126948a8409bba13bc49af75d729R25

@sbailey we should just remove this line from main and then tag 0.19.1. Let me know if you want me to do any of this.

araichoor commented 2 months ago

don t know if that s relevant or not - in which case sorry! - but I notice that there are a bunch of calls to desispec functions elsewhere in redrock: https://github.com/search?q=repo%3Adesihub%2Fredrock+desispec&type=code. i.e. the dependency on desispec will be there anyway, no?

amolaeinezhad commented 2 months ago

don t know if that s relevant or not - in which case sorry! - but I notice that there are a bunch of calls to desispec functions elsewhere in redrock: https://github.com/search?q=repo%3Adesihub%2Fredrock+desispec&type=code. i.e. the dependency on desispec will be there anyway, no?

While there are indeed calls to desispec functions within some parts of Redrock, these are primarily geared towards processing DESI spectra only. My understanding is that since version 0.16, Redrock has been fully applicable to various non-DESI projects (e.g., WEAVE, 4MOST) without the necessity to have broader DESI modules on board

sbailey commented 2 months ago

@amolaeinezhad thanks for pointing that out, and apologies for the hassle. Indeed, our intention is that any desi dependencies are isolated to redrock.external.desi (and boss by their opt-in) and the template generation code in bin/, but the core redrock algorithms should not have any desi dependencies.

I just opened PR #306 to fix this; please check if there are any other new unwanted dependencies that have crept in.

Thanks to @moustakas and @araichoor for jumping in on evaluating the situation.

amolaeinezhad commented 2 months ago

@sbailey @moustakas @araichoor Thanks for the heads up! I've checked, and it seems everything is fine now. Appreciate your diligence in addressing the issue