Dataclass for inputs#3
Conversation
…runconfig is missing fields
… dataclass_for_inputs
…nconfig to not cause errors during parsing.
gshiroma
left a comment
gshiroma
left a comment
There was a problem hiding this comment.
Thank you, Sam, for updating RunConfigConstants to dataclass. I see that you moved the parsing of the runconfig files to RunConfigConstants. I'm not really sure if there's any real advantage to that? I was expecting just the change of RunConfigConstants to dataclass because that would be fast to implement and easy to review. I honestly don't think I'll have time to review and test this PR in time for this delivery =/ Thank you also for providing some nice comments and docstrings. I have some comments below. If we don't have time to merge it on time, we'll try to do it for next release, ok?
|
|
||
|
|
||
| # Get the final runconfig dict | ||
| runconfig = runconfig_constants.get_final_runconfig_dict(quiet=True) |
There was a problem hiding this comment.
Uhm, why is this being called again? Did anything change since you called it from the constructor? Or maybe because you need a runconfig object?
There was a problem hiding this comment.
Ah, the trick is that the rest of the parse_runconfig_file() function is written such that it needs to have access to a dictionary with the same structure as the default runconfig file, but updated with user runconfig values.
An alternate option could be that during initialization, we store that dictionary as an attribute inside the RunConfigConstants object. However, this means it will persist in memory. (Albeit, it's very small so probably not a big deal.) Maybe something like that instead?
| parser.add_argument('--use-otsu-terrain-masking', | ||
| dest='flag_use_otsu_terrain_masking', | ||
| action='store_true', | ||
| default=None, | ||
| help=('Compute and apply terrain masking using Otsu' | ||
| ' thresholding')) | ||
|
|
||
| parser.add_argument('--min-slope-angle', | ||
| dest='min_slope_angle', | ||
| type=float, | ||
| help='') | ||
|
|
||
| parser.add_argument('--max-sun-local-inc-angle', | ||
| dest='max_sun_local_inc_angle', | ||
| type=float, | ||
| help='Maximum local-incidence angle') | ||
|
|
||
| parser.add_argument('--mask-adjacent-to-cloud-mode', | ||
| dest='mask_adjacent_to_cloud_mode', | ||
| type=str, | ||
| choices=['mask', 'ignore', 'cover'], | ||
| help='Define how areas adjacent to cloud/cloud-shadow' | ||
| ' should be handled. Options: "mask", "ignore", and' | ||
| ' "cover"') | ||
|
|
There was a problem hiding this comment.
Why are we removing all these parameters? As we discussed offline, we should be able to update the code to use them properly.
2 participants
This PR updates and streamlines how the constants in the user runconfig and default runconfig files are parsed and used within the code's workflow. It establishes clearly the precedence for how these default values are selected, and ensures that the proper value is selected. It also removes these constants from the CLI arguments, in order to prevent confusion from the users about where to set those values.