Multiple cfgs #739
Multiple cfgs #739
Conversation
|
Hello @awirb! Thanks for opening this PR.
Do see the Hitchhiker's guide to code style |
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| comModule='com1DFA') | ||
|
|
||
| # fetch configuration for DFAPathGeneration | ||
| hybridModelPathCfg= cfgUtils.getModuleConfig(DFAPath, fileOverride='', modInfo=False, toPrint=False, |
There was a problem hiding this comment.
Similar blocks of code found in 3 locations. Consider refactoring.
|
Code Climate has analyzed commit afa3305 and detected 6 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 22.7% (50% is the threshold). This pull request will bring the total coverage in the repository to 73.2% (0.0% change). View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## master #739 +/- ##
==========================================
- Coverage 75.58% 75.43% -0.15%
==========================================
Files 55 55
Lines 11337 11366 +29
==========================================
+ Hits 8569 8574 +5
- Misses 2768 2792 +24
Continue to review full report at Codecov.
|
| fU.makeADir(oPath) | ||
|
|
||
| # get comDFA configuration and save to file | ||
| hybridModelDFACfg = cfgUtils.getModuleConfig(com1DFA, fileOverride='', modInfo=False, toPrint=False, |
There was a problem hiding this comment.
it is maybe a bit confusing to have both a fileOverride and a overrideConfig... Maybe we could rename this a bit
There was a problem hiding this comment.
what is your suggestion? I think it is actually pretty accurate the one thing is a file path that you have to provide the other is configparser object that has the override parameters in it..
| comModule='com1DFA') | ||
|
|
||
| # fetch configuration for DFAPathGeneration | ||
| hybridModelPathCfg= cfgUtils.getModuleConfig(DFAPath, fileOverride='', modInfo=False, toPrint=False, |
There was a problem hiding this comment.
we need to come up with a good way to name these cfg objects. DFAPathcom3HybridCfg and com1DFAcom3HybridCfg and com2ABcom3HybridCfg or so no? And we always use the same method to name these files
| overrideConfig: cfg | ||
| if not empty, override configuration parameters with override parameters |
There was a problem hiding this comment.
back to my other comment, the naming is confusing, we have two override files given in input. Also update the doc string so it is clear what is happening here
| cfg.set(section, key, overrideParameters[key]) | ||
| log.info('Override parameter: %s in section: %s with %s' % (key, section, str(overrideParameters[key]))) | ||
| else: | ||
| overrideParameters[key] = cfg[section][key] |
There was a problem hiding this comment.
This updates the overrideParameters (which is the override cfg object) without you returning it as an output... I works but I am not a big fan of something happening without me knowing it. Should we change this?
If not at least mention this in the function description and add a comment here
| # get comDFA configuration and save to file | ||
| hybridModelDFACfg = cfgUtils.getModuleConfig(com1DFA, fileOverride='', modInfo=False, toPrint=False, | ||
| onlyDefault=cfgHybrid['com1DFA_override']['defaultConfig'], overrideConfig=cfgHybrid) | ||
| hybridModelDFACfgFile = cfgUtils.writeCfgFile(avalancheDir, com1DFA, hybridModelDFACfg, fileName='com1DFA_settings', filePath=oPath) |
There was a problem hiding this comment.
why write this one and not also the AB and pathGeneration cfgs to file?
|
newly opened in PR #740 |
If using different modules in a new function