Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove old picca routines [bump major] #1033

Merged
merged 12 commits into from
Aug 2, 2023
Merged

Remove old picca routines [bump major] #1033

merged 12 commits into from
Aug 2, 2023

Conversation

Waelthus
Copy link
Contributor

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
Copy link
Contributor Author

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,...}

@Waelthus Waelthus self-assigned this Jul 28, 2023
@iprafols
Copy link
Collaborator

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 Waelthus requested a review from iprafols July 28, 2023 10:01
@Waelthus
Copy link
Contributor Author

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
Copy link
Contributor Author

fitter2 is already removed here

@iprafols
Copy link
Collaborator

cool, makes sense

Copy link
Collaborator

@iprafols iprafols left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine, I see that you did not remove the bits of code from io.py and the rest. Maybe you can also remove the fitter2 folder

@Waelthus
Copy link
Contributor Author

Waelthus commented Aug 1, 2023

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?

@Waelthus Waelthus marked this pull request as ready for review August 1, 2023 15:23
@Waelthus Waelthus requested a review from iprafols August 1, 2023 15:23
@iprafols
Copy link
Collaborator

iprafols commented Aug 1, 2023

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?

@iprafols iprafols changed the title Remove old picca routines Remove old picca routines [major] Aug 1, 2023
@Waelthus
Copy link
Contributor Author

Waelthus commented Aug 1, 2023 via email

@Waelthus Waelthus changed the title Remove old picca routines [major] Remove old picca routines [bump major] Aug 2, 2023
@Waelthus Waelthus added this pull request to the merge queue Aug 2, 2023
@Waelthus
Copy link
Contributor Author

Waelthus commented Aug 2, 2023

closes #1032

Merged via the queue into master with commit 0f54b47 Aug 2, 2023
8 checks passed
@Waelthus Waelthus deleted the remove_old_picca branch February 5, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants