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

Issue/49/delight #57

Closed
wants to merge 31 commits into from
Closed

Issue/49/delight #57

wants to merge 31 commits into from

Conversation

sylvielsstfr
Copy link
Collaborator

I think it is time to proceed to do a merge of delightPZ in master.

The merging is purely functionnal. It means Delight hyper parameters are not optimized for test-DC2 dataset. I will need RAIL metrics for this or doing something else.

Note to integrate Delight with DESC it is necessary to review https://github.com/sylvielsstfr/Delight

A first step is to install the Delight branch desc_rail here https://github.com/sylvielsstfr/Delight/tree/desc_rail in
https://github.com/LSSTDESC

Then most of the work of interfacing Delight with RAIL can be found in:

https://github.com/sylvielsstfr/Delight/tree/desc_rail/interfaces/rail

where it is possible to read the README.md file.

Let me know when you are ready for presenting/introducing how all this stuff works.

Sylvie

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #57 (02d9ad3) into master (d7b7c31) will decrease coverage by 74.63%.
The diff coverage is 0.39%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #57       +/-   ##
===========================================
- Coverage   97.58%   22.94%   -74.64%     
===========================================
  Files           8       28       +20     
  Lines         331     1970     +1639     
===========================================
+ Hits          323      452      +129     
- Misses          8     1518     +1510     
Flag Coverage Δ
unittests 22.94% <0.39%> (-74.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nclude_delightPZ/calibrateTemplateMixturePriors.py 0.00% <0.00%> (ø)
...timation/algos/include_delightPZ/convertDESCcat.py 0.00% <0.00%> (ø)
...estimation/algos/include_delightPZ/delightApply.py 0.00% <0.00%> (ø)
...estimation/algos/include_delightPZ/delightLearn.py 0.00% <0.00%> (ø)
.../include_delightPZ/getDelightRedshiftEstimation.py 0.00% <0.00%> (ø)
...l/estimation/algos/include_delightPZ/libPriorPZ.py 0.00% <0.00%> (ø)
...imation/algos/include_delightPZ/makeConfigParam.py 0.00% <0.00%> (ø)
...timation/algos/include_delightPZ/processFilters.py 0.00% <0.00%> (ø)
.../estimation/algos/include_delightPZ/processSEDs.py 0.00% <0.00%> (ø)
...mation/algos/include_delightPZ/simulateWithSEDs.py 0.00% <0.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b7c31...02d9ad3. Read the comment docs.

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.

@sylvielsstfr ok very first look at it. There is a lot to digest! Would it be possible to add some basic unit tests so that we clear some of the codecov warnings, and we get to see a bit better the basic functionalities?


- python setup.py build_ext --inplace
- python setup.py install --user

Copy link
Collaborator

Choose a reason for hiding this comment

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

in DESC we are advocating to avoid --user as it is extremely fragile. If the packages are not conda installable, one should still work in a conda env and run pip within this env, without --user

- **Delight/interfaces/rail**

(To avoid too much code inside RAIL)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is probably not a good idea even if it is a lot of code. And the copying of the utils.py file in Delight, which anyway is going to be deprecated, is a red flag.

[Bands]
names: lsst_u lsst_g lsst_r lsst_i lsst_z lsst_y
directory: /Users/sylvie/MacOSX/GitHub/LSST/Delight/data/FILTERS
numCoefs: 15
Copy link
Collaborator

Choose a reason for hiding this comment

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

Problem with personal path here


[Templates]
directory: /Users/sylvie/MacOSX/GitHub/LSST/Delight/data/CWW_SEDs
names: El_B2004a Sbc_B2004a Scd_B2004a SB3_B2004a SB2_B2004a Im_B2004a ssp_25Myr_z008 ssp_5Myr_z008
Copy link
Collaborator

Choose a reason for hiding this comment

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

personal path

from interfaces.rail.convertDESCcat import convertDESCcat # convert DESC input file into Delight format
from interfaces.rail.convertDESCcat import * # convert DESC input file into Delight format
from interfaces.rail.calibrateTemplateMixturePriors import *
from interfaces.rail.getDelightRedshiftEstimation import *
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need to move most of these to RAIL

@aimalz aimalz requested a review from sschmidt23 April 9, 2021 16:54
@aimalz
Copy link
Collaborator

aimalz commented Jun 4, 2021

Are these changes being reflected in the DESC fork of Delight? I think it would be easier to make the changes there and then have RAIL just install and call from that code, no?

@sschmidt23 sschmidt23 closed this Apr 15, 2022
@aimalz aimalz deleted the issue/49/delight branch May 24, 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.

4 participants