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

Fix data model #979

Merged
merged 28 commits into from
Mar 15, 2023
Merged

Fix data model #979

merged 28 commits into from
Mar 15, 2023

Conversation

cramirezpe
Copy link
Contributor

@cramirezpe cramirezpe commented Mar 10, 2023

This standardizes output fits files for image format.

I am waiting for #977 to be merged before adding tests, as both PR need to modify test files.

  • Also data model files have to be updated, as I added the attribute "units" to the rejection_log object.
  • I need to update the code in some parts, as some fitsio reading function seeem to be case sensitive: calibration_correction.py:51

@iprafols
Copy link
Collaborator

PR #977 is already merged. I have merged master into fix_data_model so that those changes are updated here as well. Let me know when you are ready to merge this PR

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, we just need to update the tests and fix one concern before we are ready to merge

@@ -350,8 +350,11 @@ def hdu_cont(self, results):
self.get_mean_cont(Forest.log_lambda_rest_frame_grid),
self.get_mean_cont_weight(Forest.log_lambda_rest_frame_grid),
],
names=['loglam_rest', 'mean_cont', 'weight'],
names=['LOGLAM_REST', 'MEAN_CONT', 'WEIGHT'],
units=['log(Angstrom)', '10**-17 erg/(s cm2 Angstrom)', ''],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking we should load this from the Data class or the Forest class, as different surveys might use different units for the flux

Copy link
Contributor Author

@cramirezpe cramirezpe Mar 14, 2023

Choose a reason for hiding this comment

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

I have added this. Only for fluxes. At first I did it also for wavelength, but I think it makes no sense to run picca in other units (for example BAL lines are defined in Angstroms, so we would need to write an unit converter)

@iprafols iprafols merged commit 172bafe into master Mar 15, 2023
@iprafols iprafols deleted the fix_data_model branch March 15, 2023 15:51
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