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

Add option to set assessment options with auto populated configs #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ioannis-vm
Copy link
Contributor

  • Adds the option to set the seed and other assessment options in cases where DL_calculation input files (config files) are auto-populated.
  • Fixes a bug on setting the seed in the assessment options object.

# include those in the original `config`
config_ap['Applications']['DL']['ApplicationData'].pop('Options')
update_vals(
config_ap['DL']['Options'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I understand what you are doing here:

  • First, you pop the Options from the ApplicationData part of the config_ap. Why?
  • Then you update the Options under DL in the config_ap with the settings loaded from the ApplicationData part of the original config, not the config_ap.

If it was config_ap everywhere, it would make more sense. But now it is quite confusing with these two different config variables and the two different locations for Options in each.

Please help me understand.

Wouldn't we need an 'Applications' and a in there? Like so: `config_ap['Applications']['DL']['ApplicationData']['Options']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The config variable stores the DL_calculation.py input file, defined as a JSON file.
  • There are two options for this file, it can either define a DL section or not. In the latter case, auto-population is required.
  • The correct location to place the seed so that it is properly propagated to the Assessment object is DL/Options/Seed, but when auto-population is required, there is no DL section.
  • To get around that, I introduced an additional placeholder for assessment options for the case of auto-population: Applications/DL/ApplicationData/Options. Input files meant for auto-population that need to specify options can have them defined there. When the auto-populated config file is generated, it maintains that data, but we have to move them to the right location, namely DL/Options. It is possible, however, for the autopopulation script (which we have no control over) to be such that it stores values there. That's why I used the update_vals function from base, previously used to merge user-defined and default settings, to merge any auto-populated DL/Options in config_ap with those coming from Applications/DL/ApplicationData/Options. For example, if a seed value is provided both in the input file and from the auto-population script, the one from the auto-population script will take precedence.

- Adds the option to set the seed and other assessment options in
  cases where DL_calculation input files (config files) are
  auto-populated.
- Fixes a bug on setting the seed in the assessment options object.
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