made syntax to deal with parameter sub-dicts in lephare#84
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
+ Coverage 92.46% 93.45% +0.98%
==========================================
Files 2 2
Lines 146 168 +22
==========================================
+ Hits 135 157 +22
Misses 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| msg="QSO config overrides.", | ||
| ), | ||
| ) | ||
| _add_sub_config(config_options, lsst_default_config, "lephare.") |
There was a problem hiding this comment.
is there a risk of collision with the module itself : import lephare; lephare.XXX ?
There was a problem hiding this comment.
I don't think so, this only affect the keys be which we refer to parameters. I.e., if making a stage interactively you could do
param_dict = {'lephare.QSO_LIB':'LSST_QSO_BIN'}
LephareInformer.make_stage(**param_dict)
Or in configuring a stage with yaml would do:
lephare_inform:
lephare.ADAPT_BAND: '5'
The alternative would be to update the config parsing code so that it doesn't require changing the entire dict if a single parameter changes.
| ) | ||
|
|
||
| star_default_config=dict( | ||
| LIB_ASCII="YES" |
There was a problem hiding this comment.
to be revisited : LIB_ASCII set to YES on the 3 types of objects is likely not what we want for a default LSST config, if by default we mean "production ready". Same below lines 45 and 53
| LIB_ASCII="YES", | ||
| MOD_EXTINC="0,1000", | ||
| EB_V="0.,0.1,0.2,0.3", | ||
| EXTINC_LAW="SB_calzetti.dat", |
There was a problem hiding this comment.
to do : revisit choice for QSO.
| ), | ||
| ) | ||
| _add_sub_config(config_options, lsst_default_config, "lephare.") | ||
| _add_sub_config(config_options, star_default_config, "gal.") |
There was a problem hiding this comment.
ptrfox "star.", not "gal."
|
|
||
| star_config = _get_sub_config(self.config, 'star.') | ||
| gal_config = _get_sub_config(self.config, 'gal.') | ||
| qso_config = _get_sub_config(self.config, 'gso.') |
There was a problem hiding this comment.
bug : prefix 'qso.' not 'gso.'
johannct
left a comment
johannct
left a comment
There was a problem hiding this comment.
LGTM
The added benefit is that the defaults are moved outside of the functions needing them, cleaner and clearer.
My comments on revisiting defaults will be deferred to another issue/PR
4 participants
Problem & Solution Description (including issue #)
Code Quality
#pragma: no cover; in the case of a bugfix, a new test that breaks as a result of the bug has been added#80