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

Delight for RAIL #3

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open

Delight for RAIL #3

wants to merge 51 commits into from

Conversation

aimalz
Copy link
Collaborator

@aimalz aimalz commented Apr 7, 2021

This should make the inclusion of Delight in RAIL cleaner to initiate and maintain.

@johannct johannct self-requested a review April 7, 2021 19:58
Copy link
Collaborator

@johannct johannct left a comment

Choose a reason for hiding this comment

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

Thanks a lot @sylvielsstfr , I will need to install it and run it to understand the codebase better. In the meantime there are a few essentially cosmetic requests. We probably need to discuss the general interfacing to RAIL as well

@@ -0,0 +1,2 @@
global-include *.res
global-include *.dat
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a newline

@@ -148,8 +191,11 @@ def parseParamFile(fileName, verbose=True, catFilesNeeded=True):
config.get('Other', 'confidenceLevels').split(' ')]

if verbose:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is there still a verbose condition if you use a logger?

@@ -2,8 +2,14 @@

import numpy as np

from scipy.misc import logsumexp
#from scipy.misc import logsumexp
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete commented out changes

@@ -0,0 +1,263 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

add docstrings to explain this new code

@@ -0,0 +1,232 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

add docstrings to explain this new code

# if make_plot, save images
#
# output file : band + '_gaussian_coefficients.txt'
#####################################################################################################
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is there a code fitting band filters with a gaussian mixtures here and in interface/rails?

#
# output file : sed_name + '_fluxredshiftmod.txt'
######################################################################################################

Copy link
Collaborator

Choose a reason for hiding this comment

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

likewise, isn't this a duplicate?

#package_data={'delightdata': ['data/BROWN_SEDs/*.dat', 'data/CWW_SEDs/*.dat','data/FILTERS/*.res']},
package_data={'': extra_files},
#include_package_data = True,

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented out code

## Utilities

- **utils.py** : Library of IO inside RAIL used in **convertDESCcat.py** for reading DESC dataset. (It means it has been copied from RAIL to Delight bacause it is not possible to make cross dependence between these packages (Delight depending on RAIL and RAIL depending on Delight).

Copy link
Collaborator

Choose a reason for hiding this comment

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

right but copying the codebase is not a solution either. Like in the comment above, I think that the code relying on it needs to live in rail, not in Delight

- **convertDESCcat.py**: Convert DESC PhotoZ dataset into ascii files readable by Delight. For now, only files with hdf5 format are used.

- **getDelightRedshiftEstimation.py**: Retrieve results on PhotoZ estimation of Delight for RAIL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if by interface we mean here "running with rail", then this code probably needs to live in RAIL

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.

3 participants