igmhub / picca

set of tools for continuum fitting, correlation function calculation, cosmological fits...
GNU General Public License v3.0
30 stars 22 forks source link

Remove old picca routines [bump major] #1033

Closed Waelthus closed 11 months ago

Waelthus commented 11 months ago

This PR intends to remove old picca routines, such as picca_deltas and picca_fitter2 with their dependencies (but leaving the parts that correlation function and p1d still need). Adresses #1032

Waelthus commented 11 months ago

It's expected that code testing coverage will change, I'll use that as a hint regarding which parts may be removable e.g. in picca/{io.py,prep_del.py,...}

iprafols commented 11 months ago

io.py will definitely not be removable since it is also used for the computation of the correlation function. Maybe for now we can just remove picca_deltas.py and the tests associated with it. We might as well remove the fitter2 now that we're on this. We have also been using Vega for a while now

Waelthus commented 11 months ago

yes, I know that it won't be fully removable, but the other files regarding deltas will be. And for io I'd at least remove a bunch of spectra-read-in (read_data, read_from...) functions, but will keep deltas/objects/dlas/absorbers/drq alive.

Waelthus commented 11 months ago

fitter2 is already removed here

iprafols commented 11 months ago

cool, makes sense

Waelthus commented 11 months ago

Most legacy code that isn't used by for cf et al is gone. Parts of data.Forest are still there as cf hijacks it's variables, prep_del is only kept for stacking, raw_io is kept in case it's still needed (as we don't test it afaik). We might want to have a test of non-automatically tested parts of the code, e.g. dmat or wick

Which version do we want? 8.0 as this is breaking all bw compatibility to old picca?

iprafols commented 11 months ago

I'd say to go to version 8, yes. It does make sense to keep the raw analysis for the meantime (maybe we should try to migrate it to be part of the delta_extraction code at some point. Also, I totally agree on having tests for the missing parts.

I see that the test suit has changed. Was this needed due to the library changes that made the tests fail?

Waelthus commented 11 months ago

Ah, I originally thought I could remove everything related to picca_deltas tests. Just to figure out later that those files were partially inputs of the cf and p1d tests, so one commit needed partial revert... In the end it should be removals of files and code only (and changing some comments).

Am 1. August 2023 17:25:08 MESZ schrieb "Ignasi Pérez-Ràfols" @.***>:

I'd say to go to version 8, yes. It does make sense to keep the raw analysis for the meantime (maybe we should try to migrate it to be part of the delta_extraction code at some point. Also, I totally agree on having tests for the missing parts.

I see that the test suit has changed. Was this needed due to the library changes that made the tests fail?

-- Reply to this email directly or view it on GitHub: https://github.com/igmhub/picca/pull/1033#issuecomment-1660554435 You are receiving this because you were assigned.

Message ID: @.***>

Waelthus commented 11 months ago

closes #1032